Feature/hash failure domain naming #147

rejoshed · 2022-08-10T20:48:30Z

Issue #, if available:

Description of changes:
Meta naming for failure domain custom resource objects currently follows failureDomainName + - + clusterName.

This was to work around EKS-A's decision to put all clusters in a single namespace.

Unfortunately k8s object names can only be 64 characters long and this leads to the concern that CAPC will break for any combination of names that leads to >64 character failure domain naming.

This uses a hash of the failure domain name and the cluster name to predictably generate a failure domain's meta name.

Also should close #113 (comment)

Testing performed:
All unit tests passing locally.

Ran e2e PR-Blocking tests successfully.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

k8s-ci-robot · 2022-08-10T20:48:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2022-08-10T20:48:37Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!

Name	Link
🔨 Latest commit	`9f10f03`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-cloudstack/deploys/62f5427f2df2da00080bb97e
😎 Deploy Preview	https://deploy-preview-147--kubernetes-sigs-cluster-api-cloudstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

rejoshed · 2022-08-10T20:49:58Z

api/v1beta2/cloudstackmachine_types.go

@@ -63,7 +63,7 @@ type CloudStackMachineSpec struct {

 	// Optional affinitygroupids for deployVirtualMachine
 	// +optional
-	AffinityGroupIDs []string `json:"affinitygroupids,omitempty"`
+	AffinityGroupIDs []string `json:"affinityGroupIDs,omitempty"`


Just a fixup for JSON tagging consistency.

rejoshed · 2022-08-10T20:51:56Z

controllers/cloudstackfailuredomain_controller.go


 	// Check if the passed network was an isolated network or the network was missing. In either case, create a
 	// CloudStackIsolatedNetwork to manage the many intricacies and wait until CloudStackIsolatedNetwork is ready.
 	if r.ReconciliationSubject.Spec.Zone.Network.ID == "" ||
 		r.ReconciliationSubject.Spec.Zone.Network.Type == infrav1.NetworkTypeIsolated {
 		netName := r.ReconciliationSubject.Spec.Zone.Network.Name
-		if res, err := r.GenerateIsolatedNetwork(netName)(); r.ShouldReturn(res, err) {
+		if res, err := r.GenerateIsolatedNetwork(
+			netName, func() string { return r.ReconciliationSubject.Spec.Name })(); r.ShouldReturn(res, err) {


Need to pass failure domain name in here.

config/manager/manager.yaml

test/e2e/config/cloudstack.yaml

codecov-commenter · 2022-08-10T23:20:39Z

Codecov Report

Merging #147 (9f10f03) into main (a541d71) will decrease coverage by 0.18%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
- Coverage   24.35%   24.17%   -0.19%     
==========================================
  Files          43       43              
  Lines        3798     3748      -50     
==========================================
- Hits          925      906      -19     
+ Misses       2727     2699      -28     
+ Partials      146      143       -3

Impacted Files	Coverage Δ
api/v1beta1/zz_generated.conversion.go	`0.53% <0.00%> (+0.02%)`	⬆️
api/v1beta2/cloudstackisolatednetwork_types.go	`28.57% <ø> (ø)`
api/v1beta2/cloudstackmachine_types.go	`28.57% <ø> (ø)`
api/v1beta2/zz_generated.deepcopy.go	`12.29% <ø> (+0.02%)`	⬆️
...ontrollers/cloudstackisolatednetwork_controller.go	`8.51% <0.00%> (-33.49%)`	⬇️
...ollers/cloudstackmachinestatechecker_controller.go	`0.00% <0.00%> (ø)`
controllers/cloudstackfailuredomain_controller.go	`32.94% <50.00%> (+0.75%)`	⬆️
controllers/cloudstackcluster_controller.go	`44.33% <60.00%> (+0.94%)`	⬆️
api/v1beta2/cloudstackfailuredomain_types.go	`100.00% <100.00%> (ø)`
controllers/cloudstackaffinitygroup_controller.go	`64.10% <100.00%> (-0.90%)`	⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

maxdrib · 2022-08-11T00:34:31Z

api/v1beta2/cloudstackmachine_types.go

@@ -63,7 +63,7 @@ type CloudStackMachineSpec struct {

 	// Optional affinitygroupids for deployVirtualMachine
 	// +optional
-	AffinityGroupIDs []string `json:"affinitygroupids,omitempty"`
+	AffinityGroupIDs []string `json:"affinityGroupIDs,omitempty"`


I don't think we should change this now. EKS-A is already and other customers may also be using this field and changing the serialization/deserialization patterns feels risky, with no gain. I'm inclined to keep these as they are and fix later on in an intentional way

Unless this is strictly for the v1beta2 types, and not v1beta1? Same for the other changes involving json

Indeed, this is only on v1beta2 types.

That's why I wanted to jamb this into this release. It won't be easy to do outside of an API version bump.

controllers/utils/failuredomains.go

api/v1beta2/cloudstackisolatednetwork_types.go

maxdrib

Why are all these test data files deleted?

wongni · 2022-08-11T00:44:19Z

Why are all these test data files deleted?

They are generated files via kustomize during executing run-e2e make target

jweite-amazon · 2022-08-11T17:22:42Z

/approved

k8s-ci-robot · 2022-08-11T17:57:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maxdrib, rejoshed, wongni

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maxdrib,rejoshed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Joshua Reed added 2 commits August 10, 2022 12:22

Working but needs cleanup.

d932f81

Standardize json tagging naming conventions.

e33e7ea

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 10, 2022

k8s-ci-robot requested review from davidjumani and jweite-amazon August 10, 2022 20:48

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 10, 2022

rejoshed commented Aug 10, 2022

View reviewed changes

Joshua Reed added 3 commits August 10, 2022 14:43

Standardize json tagging naming conventions.

44d024b

Pr cleanup.

7e31e60

Test fixups.

1e3310d

rejoshed requested review from maxdrib and wongni and removed request for davidjumani and jweite-amazon August 10, 2022 23:04

Merge main.

ad369be

wongni reviewed Aug 10, 2022

View reviewed changes

config/manager/manager.yaml Outdated Show resolved Hide resolved

test/e2e/config/cloudstack.yaml Outdated Show resolved Hide resolved

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2022

Joshua Reed added 2 commits August 10, 2022 16:15

Cleanup.

3f5b5ba

Cleanup.

b9d5e4e

rejoshed marked this pull request as ready for review August 10, 2022 23:18

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2022

k8s-ci-robot requested a review from jweite-amazon August 10, 2022 23:19

Cleanup files that shouldn't really be checked in anyway.

9b151fb

Fixup makefile minor issue with e2e template directory.

b336959

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 11, 2022

rejoshed requested a review from wongni August 11, 2022 00:04

wongni approved these changes Aug 11, 2022

View reviewed changes

maxdrib reviewed Aug 11, 2022

View reviewed changes

Cleanup.

6c41e57

Update acsEndpoint JSON again.

9f10f03

maxdrib approved these changes Aug 11, 2022

View reviewed changes

rejoshed merged commit 4a47522 into kubernetes-sigs:main Aug 11, 2022

rejoshed deleted the feature/hash-failure-domain-naming branch August 11, 2022 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/hash failure domain naming #147

Feature/hash failure domain naming #147

Uh oh!

rejoshed commented Aug 10, 2022 •

edited

Loading

Uh oh!

k8s-ci-robot commented Aug 10, 2022

Uh oh!

netlify bot commented Aug 10, 2022 •

edited

Loading

Uh oh!

rejoshed Aug 10, 2022

Uh oh!

rejoshed Aug 10, 2022

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 10, 2022 •

edited

Loading

Uh oh!

maxdrib Aug 11, 2022

Uh oh!

rejoshed Aug 11, 2022

Uh oh!

Uh oh!

Uh oh!

maxdrib left a comment

Uh oh!

wongni commented Aug 11, 2022

Uh oh!

jweite-amazon commented Aug 11, 2022

Uh oh!

k8s-ci-robot commented Aug 11, 2022

Uh oh!

Uh oh!

Feature/hash failure domain naming #147

Feature/hash failure domain naming #147

Uh oh!

Conversation

rejoshed commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 10, 2022

Uh oh!

netlify bot commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!

Uh oh!

rejoshed Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

rejoshed Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

maxdrib Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

rejoshed Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maxdrib left a comment

Choose a reason for hiding this comment

Uh oh!

wongni commented Aug 11, 2022

Uh oh!

jweite-amazon commented Aug 11, 2022

Uh oh!

k8s-ci-robot commented Aug 11, 2022

Uh oh!

Uh oh!

rejoshed commented Aug 10, 2022 •

edited

Loading

netlify bot commented Aug 10, 2022 •

edited

Loading

codecov-commenter commented Aug 10, 2022 •

edited

Loading