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.

Sign up for GitHub

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

Jump to bottom

Refactor away from using Cluster object in nodeup #14870

Merged
merged 5 commits into from
Jan 2, 2023
Merged

Refactor away from using Cluster object in nodeup #14870

merged 5 commits into from
Jan 2, 2023

Conversation

johngmyers
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 25, 2022
@k8s-ci-robot k8s-ci-robot added area/api area/nodeup area/provider/aws Issues or PRs related to aws provider area/provider/gcp Issues or PRs related to gcp provider area/rolling-update labels Dec 25, 2022
@johngmyers
Copy link
Member Author

/retest

2 similar comments
@johngmyers
Copy link
Member Author

/retest

@johngmyers
Copy link
Member Author

/retest

@johngmyers johngmyers changed the title Refactor away from using Cluster object in nodeup WIP Refactor away from using Cluster object in nodeup Dec 25, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 25, 2022
@johngmyers johngmyers force-pushed the nodeup-clustername branch 2 times, most recently from d8896b4 to ce85efa Compare December 25, 2022 20:47
@johngmyers
Copy link
Member Author

/retest

1 similar comment
@johngmyers
Copy link
Member Author

/retest

@johngmyers johngmyers changed the title WIP Refactor away from using Cluster object in nodeup Refactor away from using Cluster object in nodeup Dec 25, 2022
@k8s-ci-robot 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 Dec 25, 2022
@johngmyers
Copy link
Member Author

/retest

@johngmyers
Copy link
Member Author

/retest

1 similar comment
@johngmyers
Copy link
Member Author

/retest

@johngmyers
Copy link
Member Author

@justinsb I can't quite figure out the gce-cilium e2e failure. dns-controller can't connect to Gossip. It's possibly related to #14877.

@johngmyers
Copy link
Member Author

johngmyers commented Dec 26, 2022 •
edited
Loading

Ah, my change had broken Gossip because nodeup was checking the Name field of the Cluster object.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2022
hakman
hakman reviewed Dec 30, 2022
View reviewed changes
nodeup/pkg/model/context.go
Comment on lines +72 to +74
// IsGossip is true if the cluster runs Gossip DNS.
IsGossip bool

Copy link
Member

Choose a reason for hiding this comment

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

I dislike the IsGossip name. Maybe UsesGossipDNS? Though... let's say it's consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also lean towards a function if we can determine it from other values, but ... I don't think it's a blocker as it's only internal implementation (I believe)

nodeup/pkg/model/etc_hosts.go Outdated
task.Records = append(task.Records, nodetasks.HostRecord{
Hostname: b.Cluster.APIInternalName(),
Hostname: "api.internal." + b.NodeupConfig.ClusterName,
Copy link
Member

Choose a reason for hiding this comment

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

It's pretty hard already tracking all instances of using "internal" names. Best to keep this as a function or variable.

@johngmyers
Copy link
Member Author

/retest

justinsb
justinsb reviewed Dec 30, 2022
View reviewed changes
nodeup/pkg/model/context.go Outdated
@@ -325,7 +334,7 @@ func (c *NodeupModelContext) BuildBootstrapKubeconfig(name string, ctx *fi.Nodeu
// This code path is used for the kubelet cert in Kubernetes 1.18 and earlier.
kubeConfig.ServerURL = "https://127.0.0.1"
} else {
kubeConfig.ServerURL = "https://" + c.Cluster.APIInternalName()
kubeConfig.ServerURL = "https://api.internal." + c.NodeupConfig.ClusterName
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to keep the function here (c.APIInternalName()), though not a blocker.

nodeup/pkg/model/etc_hosts.go
Addresses: []string{b.BootConfig.APIServerIP},
})
if b.UseKopsControllerForNodeBootstrap() {
task.Records = append(task.Records, nodetasks.HostRecord{
Hostname: "kops-controller.internal." + b.Cluster.Name,
Hostname: "kops-controller.internal." + b.NodeupConfig.ClusterName,
Copy link
Member

Choose a reason for hiding this comment

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

I see hakman had the same idea about functions above - maybe we make this a function as well, as it's otherwise inconsistent (albeit pre-existing inconsistency). Not a blocker though, as this is internal / can be done later.

@justinsb
Copy link
Member

Generally lgtm - and I really like the direction of the refactor. A few quibbles over when we should use a function, but we can maybe reach some guidelines in office hours (if we're having them today? If not I vote to merge and iterate, I might propose a few helper functions for the hostnames in particular).

I really like the direction here though, I would love to have nodeup only work in terms of Tasks that come from kops-controller (or from metadata in the case of control plane). That would require us to treat (nodeup) Tasks as versioned, but I think it would be great to simplify nodeup and enable things like "hot" upgrades. But that's more directional IMO!

@johngmyers
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 1, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 1, 2023
@johngmyers
Copy link
Member Author

Is there anything else for this PR?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2023
@k8s-ci-robot k8s-ci-robot merged commit ac6d623 into kubernetes:master Jan 2, 2023
@johngmyers johngmyers deleted the nodeup-clustername branch January 2, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@hakman hakman hakman approved these changes

@olemarkus olemarkus Awaiting requested review from olemarkus

Assignees

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/nodeup area/provider/aws Issues or PRs related to aws provider area/provider/gcp Issues or PRs related to gcp provider area/rolling-update cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johngmyers @justinsb @k8s-ci-robot @hakman