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

support multiple ConfigServers #15000

Merged
merged 2 commits into from
Jan 17, 2023
Merged

support multiple ConfigServers #15000

merged 2 commits into from
Jan 17, 2023

Conversation

zetaab
Copy link
Member

@zetaab zetaab commented Jan 14, 2023

Instead of supporting one boostrap ConfigServer, I modified it to understand multiple ConfigServers. The logic is that it will try to find the working one from the all available configservers. It seems that Kops-controller will answer only from one master

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2023
@k8s-ci-robot k8s-ci-robot added area/nodeup size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2023
zetaab
zetaab commented Jan 14, 2023
View reviewed changes
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
hakman
hakman requested changes Jan 14, 2023
View reviewed changes
Copy link
Member

@hakman hakman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I generally like the direction. Leaving the final review to @justinsb or @johngmyers.
/assign @johngmyers
/assign @justinsb

pkg/apis/nodeup/config.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
@johngmyers
Copy link
Member

I'll have to research why it only answers from one instance.

@johngmyers
Copy link
Member

Okay, this was introduced in #12997

The server should not be put under control of the manager, since that subjects it to leader election.

@johngmyers
Copy link
Member

@zetaab I'd appreciate if you could try #15002.

@hakman
Copy link
Member

hakman commented Jan 14, 2023

I think without putting all 3 servers (when available) there is less redundancy. We already send all 3 IPs for the bootstrap check.
Also, seems this is the direction in which @justinsb was going:
#14501 (comment)
#14769 (comment)

johngmyers
johngmyers requested changes Jan 14, 2023
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

Next time, when both the change and the changes to golden outputs are nontrivial please put the golden output changes in a separate commit.

pkg/apis/nodeup/config.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
@zetaab
Copy link
Member Author

zetaab commented Jan 15, 2023

/retest

johngmyers
johngmyers approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

/hold to give opportunity to fix nits

pkg/apis/nodeup/config.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 16, 2023
@johngmyers
Copy link
Member

/lgtm cancel
/approve cancel

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 16, 2023
@johngmyers
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 16, 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 16, 2023
hakman
hakman requested changes Jan 16, 2023
View reviewed changes
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
upup/pkg/fi/nodeup/command.go Outdated Show resolved Hide resolved
@hakman
Copy link
Member

hakman commented Jan 16, 2023

Please restage/squash the commits before merging. Will be easier to cherrypick.
/lgtm

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

hakman commented Jan 16, 2023

I meant to keep all code in one PR and the golden outputs, as @johngmyers said.
When we cherry-pick, most likely the code will not have any changes, but the expected will be regenerated. It's much easier later to track if a commit was ported to a branch or not.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 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 17, 2023
@k8s-ci-robot k8s-ci-robot merged commit 98b8c01 into kubernetes:master Jan 17, 2023
@hakman hakman mentioned this pull request Jan 17, 2023
Merged
@zetaab zetaab deleted the feature/multiconfigserver branch January 17, 2023 16:41
k8s-ci-robot added a commit that referenced this pull request Jan 17, 2023
@k8s-ci-robot
Merge pull request #15018 from hakman/automated-cherry-pick-of-#15000…
5fda685 
…-upstream-release-1.26

Automated cherry pick of #15000: support multiple ConfigServers
Shimiazoulai pushed a commit to spotinst/kubernetes-kops that referenced this pull request Jul 13, 2023
@k8s-ci-robot
Merge pull request kubernetes#15018 from hakman/automated-cherry-pick…
ffb89ce 
…-of-#15000-upstream-release-1.26

Automated cherry pick of kubernetes#15000: support multiple ConfigServers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers approved these changes

@hakman hakman hakman approved these changes

Assignees

@justinsb justinsb

@johngmyers johngmyers

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/nodeup 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

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