Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rudoi/honor machinedeployments in machine phase #3

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rudoi
Copy link

@rudoi rudoi commented Apr 5, 2019

What this PR does / why we need it:

The apply-machines phase will now detect and apply MachineDeployments as nodes, and therefore, so will clusterctl create cluster.

Release note:

Support MachineDeployments as part of the apply-machines phase

@rudoi rudoi requested a review from h0tbird April 5, 2019 15:52
Copy link

@h0tbird h0tbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware that CAPA requires a change too to be compatible with this code.
https://github.com/newrelic-forks/cluster-api-provider-aws/blob/6f304f5e2d2c51e16d608797bba790e0306b4787/pkg/cloud/aws/services/kubeadm/aws_defaults.go#L169

➜  cluster-api-provider-aws git:(test-andrew-pr) ✗ git diff
diff --git a/pkg/cloud/aws/services/kubeadm/aws_defaults.go b/pkg/cloud/aws/services/kubeadm/aws_defaults.go
index 9721c874..34da098d 100644
--- a/pkg/cloud/aws/services/kubeadm/aws_defaults.go
+++ b/pkg/cloud/aws/services/kubeadm/aws_defaults.go
@@ -166,7 +166,7 @@ func SetJoinNodeConfigurationOverrides(caCertHash, bootstrapToken string, machin
                klog.Infof("Overriding node's cloud-provider to the required value of %q.", cloudProvider)
        }
        base.NodeRegistration.KubeletExtraArgs["cloud-provider"] = cloudProvider
-       if !util.IsControlPlaneMachine(machine.Machine) {
+       if !util.IsControlPlaneMachine(machine.Machine.Spec) {
                base.NodeRegistration.KubeletExtraArgs["node-labels"] = nodeRole
        }
 }

@rudoi
Copy link
Author

rudoi commented Apr 5, 2019

@h0tbird I created a new function specific to MachineDeployments as to not break that function.

Copy link

@h0tbird h0tbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo

@rudoi rudoi force-pushed the rudoi/honor-machinedeployments-in-machine-phase branch from f789e12 to e1c077c Compare April 5, 2019 22:46
Copy link

@h0tbird h0tbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A MachineDeployment cannot be used to coordinate the init and join phases.
I think we should be able to use a MachineDeployment for all the join nodes after the pivot takes place.

cmd/clusterctl/clusterdeployer/clusterdeployer.go Outdated Show resolved Hide resolved
Copy link

@h0tbird h0tbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rudoi rudoi force-pushed the rudoi/honor-machinedeployments-in-machine-phase branch from 3a85cc0 to 6a27382 Compare April 15, 2019 20:55
sethp-nr pushed a commit that referenced this pull request Feb 29, 2020
w21froster pushed a commit that referenced this pull request Sep 19, 2024
* Introduce CEL for ClusterClass Variables

Signed-off-by: chaunceyjiang <[email protected]>

* feat: Implement CEL validation

* refactor: Add comments from previous code reviews

* chore: Generate CC manifest after fixing list type annotation (#2)

* chore: Fix up CRD manifest

* fix: Pass through context to CEL funcs

* feat: Add CEL admission cost validation

* refactor: Add nolint to unbounded

* refactor: Fix up new func signature

* build: Fix up go mod for tools

* fixup! refactor: Apply review feedback

* fixup! build: Regenerate openapi spec

* fixup! refactor: Apply review feedback

* fixup! fix: Regenerate everything

* fixup! fix: Apply review feedback

* fixup! fix: More review feedback

* fixup! refactor: Address review feedback, especially re recursion

* fixup! fix: Check total cost

* fixup! refactor: Address review feedback - rename testCtx to ctx

* CEL: Various improvements (#3)

* resolve compile issue after rebase

* Some more improvements (#4)

---------

Signed-off-by: chaunceyjiang <[email protected]>
Co-authored-by: Jimmi Dyson <[email protected]>
Co-authored-by: Jimmi Dyson <[email protected]>
Co-authored-by: Stefan Büringer <[email protected]>
Co-authored-by: Stefan Bueringer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants