feat(net): Add multizone deployment capabilities to subnets #276

gvdhart · 2023-10-18T13:32:30Z

What type of PR is this?

/kind feature New functionality with test.

What this PR does / why we need it:

This PR implements the multi az feature for subnets. Subnets that are deployed with cluster-api can now be defined with a SubregionName instead of defaulting to the OscNetwork SubregionName.
In order to be fully resilient the NAT services can also be deployed in multi az's in this PR.

TODOs:

squashed commits
includes documentation
adds unit tests

ghost · 2023-10-18T16:23:56Z

You have to keep a compatibility with user which are currently used network.subregionName.
For example, if network.subregionName exist, you will use this subregion. If you have your subregionName in your subnet you will use this one ?
What do you think ?

ghost · 2023-10-18T16:30:36Z

Also you can also add subnet.subregionName here in https://github.com/outscale/cluster-api-provider-outscale/blob/main/api/v1beta1/oscmachine_validation.go the subregionName,

(Like in osc_machine https://github.com/outscale/cluster-api-provider-outscale/blob/main/api/v1beta1/oscmachine_validation.go )

ghost · 2023-10-18T16:36:31Z

Also please add a example with multi subnet region.

ghost · 2023-10-18T16:37:55Z

Also you can also migrate all the template which are used in CI https://github.com/outscale/cluster-api-provider-outscale/blob/main/test/e2e/data/infrastructure-outscale

api/v1beta1/types.go

api/v1beta1/osccluster_validation.go

testenv/osccluster_controller_test.go

ghost · 2023-10-18T16:42:06Z

And also in template ?
(https://github.com/outscale/cluster-api-provider-outscale/tree/main/templates)

gvdhart · 2023-10-23T09:32:20Z

You have to keep a compatibility with user which are currently used network.subregionName. For example, if network.subregionName exist, you will use this subregion. If you have your subregionName in your subnet you will use this one ? What do you think ?

Yes i agree, i will do just that!

gvdhart · 2023-10-24T11:28:39Z

Also you can also add subnet.subregionName here in https://github.com/outscale/cluster-api-provider-outscale/blob/main/api/v1beta1/oscmachine_validation.go the subregionName,

(Like in osc_machine https://github.com/outscale/cluster-api-provider-outscale/blob/main/api/v1beta1/oscmachine_validation.go )

I don't understand what you mean here.
And i think as i made it fully backwards compatible I no longer need to migrate all templates for the CI

cloud/services/net/mock_net/subnet_mock.go

api/v1beta1/osccluster_validation.go

cloud/services/net/subnet.go

ghost · 2023-10-25T14:34:08Z

And please rebase from main branch (because some omi have changed)

ghost · 2023-10-25T14:38:17Z

(Ps i use tilt (which is recommand in cluster-api https://cluster-api.sigs.k8s.io/developer/tilt.html?highlight=Tilt#customizing-tilt ) and vscode to dev cluster-api (https://github.com/outscale/cluster-api-provider-outscale/blob/main/docs/src/developers/developement.md )

api/v1beta1/types.go

controllers/osccluster_subnet_controller_unit_test.go

ghost · 2023-10-25T15:02:43Z

in osccluster_controller_test.go (osc_region and osc_subregion) is
not use in this section of test

		It("should create a simple cluster with default values", func() {
			ctx := context.Background()
			osc_region, ok := os.LookupEnv("OSC_REGION")
			if !ok {
				osc_region = "eu-west-2"
			}
			osc_subregion, ok := os.LookupEnv("OSC_SUBREGION_NAME")
			if !ok {
				osc_subregion = osc_region + "a"
			}

example/cluster-machine-template-multi-az.yaml

apicovers.html

apicovers.txt

covers.html

covers.txt

ghost · 2023-11-27T13:52:48Z

@gvdhart @sebglon
Can you please write a message (or something else) when your PR is ready and it is ok to test and review for each of your PR ?
Thanks by advance :)

sebglon · 2023-11-28T09:14:18Z

@outscale-vbr this MR is ready to review

ghost · 2023-11-29T09:41:04Z

@sebglon @gvdhart
In order to fix boilerplate:

outscale@ip-10-9-39-194:~/test/cluster-api-provider-outscale$ make verify-boilerplate
GOPATH=/home/outscale/go ./hack/verify-boilerplate.sh

ROOT_PATH=$(git rev-parse --show-toplevel)

boilerDir="${ROOT_PATH}/hack/boilerplate"
boiler="${boilerDir}/boilerplate.py"

run_boilerplate(){
	boilerplate_file=()
	while IFS=$'\n' read -r line; do
	  boilerplate_file+=( "$line" )
	done < <("${boiler}" "$@")

	if [[ ${#boilerplate_file[@]} -gt 0 ]]; then
	  for file in "${boilerplate_file[@]}"; do
   	    echo "Boilerplate header is wrong for: ${file}" >&2
  	done
  exit 1
fi
}

run_boilerplate "$@"
Boilerplate header is wrong for: /home/outscale/test/cluster-api-provider-outscale/cloud/scope/mock_scope/cluster_mock.go
make: *** [Makefile:476: verify-boilerplate] Error 1

Please ignore your mock:

outscale@ip-10-9-39-194:~/test/cluster-api-provider-outscale$ git diff 
diff --git a/hack/boilerplate/boilerplate.py b/hack/boilerplate/boilerplate.py
index cb0093f..6326161 100755
--- a/hack/boilerplate/boilerplate.py
+++ b/hack/boilerplate/boilerplate.py
@@ -151,7 +151,7 @@ skipped_dirs = ['Godeps', 'third_party', '_gopath', '_output', '.git', 'cluster/
                 "pkg/kubectl/generated/bindata.go", "tilt_modules", "cloud/services/net/mock_net",
                 "cloud/services/security/mock_security", "cloud/services/service/mock_service",
                 "cloud/services/storage/mock_storage", "cloud/services/compute/mock_compute",
-                "cloud/tag/mock_tag", "api/v1beta1/zz_generated.deepcopy.go"]
+                "cloud/tag/mock_tag", "/cloud/scope/mock_scope", "api/v1beta1/zz_generated.deepcopy.go"]
 
 # list all the files contain 'DO NOT EDIT', but are not generated
 skipped_ungenerated_files = ['hack/lib/swagger.sh', 'hack/boilerplate/boilerplate.py']

DvdChe · 2023-11-29T14:43:45Z

@sebglon @gvdhart In order to fix boilerplate:

outscale@ip-10-9-39-194:~/test/cluster-api-provider-outscale$ make verify-boilerplate
GOPATH=/home/outscale/go ./hack/verify-boilerplate.sh

ROOT_PATH=$(git rev-parse --show-toplevel)

boilerDir="${ROOT_PATH}/hack/boilerplate"
boiler="${boilerDir}/boilerplate.py"

run_boilerplate(){
	boilerplate_file=()
	while IFS=$'\n' read -r line; do
	  boilerplate_file+=( "$line" )
	done < <("${boiler}" "$@")

	if [[ ${#boilerplate_file[@]} -gt 0 ]]; then
	  for file in "${boilerplate_file[@]}"; do
   	    echo "Boilerplate header is wrong for: ${file}" >&2
  	done
  exit 1
fi
}

run_boilerplate "$@"
Boilerplate header is wrong for: /home/outscale/test/cluster-api-provider-outscale/cloud/scope/mock_scope/cluster_mock.go
make: *** [Makefile:476: verify-boilerplate] Error 1

Please ignore your mock:

outscale@ip-10-9-39-194:~/test/cluster-api-provider-outscale$ git diff 
diff --git a/hack/boilerplate/boilerplate.py b/hack/boilerplate/boilerplate.py
index cb0093f..6326161 100755
--- a/hack/boilerplate/boilerplate.py
+++ b/hack/boilerplate/boilerplate.py
@@ -151,7 +151,7 @@ skipped_dirs = ['Godeps', 'third_party', '_gopath', '_output', '.git', 'cluster/
                 "pkg/kubectl/generated/bindata.go", "tilt_modules", "cloud/services/net/mock_net",
                 "cloud/services/security/mock_security", "cloud/services/service/mock_service",
                 "cloud/services/storage/mock_storage", "cloud/services/compute/mock_compute",
-                "cloud/tag/mock_tag", "api/v1beta1/zz_generated.deepcopy.go"]
+                "cloud/tag/mock_tag", "/cloud/scope/mock_scope", "api/v1beta1/zz_generated.deepcopy.go"]
 
 # list all the files contain 'DO NOT EDIT', but are not generated
 skipped_ungenerated_files = ['hack/lib/swagger.sh', 'hack/boilerplate/boilerplate.py']

We just tried to reproduce this error but we did not succeed :

GOPATH=/Users/dch/go ./hack/verify-boilerplate.sh

ROOT_PATH=$(git rev-parse --show-toplevel)
git rev-parse --show-toplevel

boilerDir="${ROOT_PATH}/hack/boilerplate"
boiler="${boilerDir}/boilerplate.py"

run_boilerplate(){
        boilerplate_file=()
        while IFS=$'\n' read -r line; do
          boilerplate_file+=( "$line" )
        done < <("${boiler}" "$@")

        if [[ ${#boilerplate_file[@]} -gt 0 ]]; then
          for file in "${boilerplate_file[@]}"; do
            echo "Boilerplate header is wrong for: ${file}" >&2
        done
  exit 1
fi
}

run_boilerplate "$@"
"${boiler}" "$@"

ghost · 2023-11-30T13:07:53Z

testenv/osccluster_controller_test.go

@@ -890,17 +891,8 @@ var _ = Describe("Outscale Cluster Reconciler", func() {
 		})
 		It("should create a simple cluster with default values", func() {
 			ctx := context.Background()
-			osc_region, ok := os.LookupEnv("OSC_REGION")


Please set it back osc_region and osc_subregion for ci

@DvdChe @sebglon @gvdhart Add also:

if !ok { osc_region = "eu-west-2" } osc_subregion, ok := os.LookupEnv("OSC_SUBREGION_NAME") if !ok { osc_subregion = osc_region + "a" }

I put back this line but go trig a warning causes they're unused

Put it back this

SubregionName: osc_subregion (line 903)

ghost · 2023-11-30T13:10:14Z

@gvdhart @sebglon
Can you also add it failed on ci and locally otherwise :

diff --git a/hack/boilerplate/boilerplate.py b/hack/boilerplate/boilerplate.py
index cb0093f..6326161 100755
--- a/hack/boilerplate/boilerplate.py
+++ b/hack/boilerplate/boilerplate.py
@@ -151,7 +151,7 @@ skipped_dirs = ['Godeps', 'third_party', '_gopath', '_output', '.git', 'cluster/
                 "pkg/kubectl/generated/bindata.go", "tilt_modules", "cloud/services/net/mock_net",
                 "cloud/services/security/mock_security", "cloud/services/service/mock_service",
                 "cloud/services/storage/mock_storage", "cloud/services/compute/mock_compute",
-                "cloud/tag/mock_tag", "api/v1beta1/zz_generated.deepcopy.go"]
+                "cloud/tag/mock_tag", "/cloud/scope/mock_scope", "api/v1beta1/zz_generated.deepcopy.go"]
 
 # list all the files contain 'DO NOT EDIT', but are not generated
 skipped_ungenerated_files = ['hack/lib/swagger.sh', 'hack/boilerplate/boilerplate.py']

Ps it test your PR locally and with ci with those changes.
(#291)

So after your PR will be merged.

ghost · 2023-12-01T15:06:43Z

testenv/osccluster_controller_test.go

 			infraClusterSpec := infrastructurev1beta1.OscClusterSpec{
 				Network: infrastructurev1beta1.OscNetwork{
-					SubregionName: osc_subregion,


ghost reviewed Oct 18, 2023

View reviewed changes

api/v1beta1/types.go Outdated Show resolved Hide resolved

api/v1beta1/osccluster_validation.go Outdated Show resolved Hide resolved

testenv/osccluster_controller_test.go Outdated Show resolved Hide resolved

gvdhart force-pushed the feat/net/implement-multi-az-support branch from 60d7d5b to bc97a16 Compare October 24, 2023 11:57