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

Check if the kube-proxy VIP was already reserved #5705

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

manuelbuil
Copy link
Contributor

Proposed Changes

This PR adds a check before reserving the IP for kube-proxy. It checks if that reservation was already made and it that case, uses that IP.

Types of Changes

Bugfix

Verification

Issue explains how to verify

Testing

Linked Issues

#5704

User-Facing Change

Flannel in windows checks if a VIP was already reserved

Further Comments

@manuelbuil manuelbuil requested a review from a team as a code owner April 11, 2024 15:23
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

nits

if err != nil {
return "", fmt.Errorf("failed to create host-local store: %w", err)
}
ips := hostlocalStore.GetByID("kube-proxy", "source-vip")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ips := hostlocalStore.GetByID("kube-proxy", "source-vip")
ips := hostlocalStore.GetByID(HostlocalContainerID, HostlocalInterfaceName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm dumb. Thanks Brad

Comment on lines 128 to 134
FlannelConfigName = "07-flannel.conflist"
FlannelKubeConfigName = "flannel.kubeconfig"
FlanneldConfigName = "flanneld-net-conf.json"
FlannelChart = "rke2-flannel"
HostlocalContainerID = "kube-proxy"
HostlocalInterfaceName = "source-vip"
HostLocalDataDir = "/var/lib/cni/networks"
Copy link
Member

Choose a reason for hiding this comment

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

Do any of these actually need to be exported? I don't see that they are used outside this package. If not, don't export them.

Suggested change
FlannelConfigName = "07-flannel.conflist"
FlannelKubeConfigName = "flannel.kubeconfig"
FlanneldConfigName = "flanneld-net-conf.json"
FlannelChart = "rke2-flannel"
HostlocalContainerID = "kube-proxy"
HostlocalInterfaceName = "source-vip"
HostLocalDataDir = "/var/lib/cni/networks"
flannelConfigName = "07-flannel.conflist"
flannelKubeConfigName = "flannel.kubeconfig"
flanneldConfigName = "flanneld-net-conf.json"
flannelChart = "rke2-flannel"
hostlocalContainerID = "kube-proxy"
hostlocalInterfaceName = "source-vip"
hostLocalDataDir = "/var/lib/cni/networks"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, I thought constants start always with a capital letter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FlannelChart is the only one used by another package

Copy link
Member

Choose a reason for hiding this comment

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

anything uppercase (const, var, or func) is exported. Anything lowercase is package local.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.04%. Comparing base (c292bb9) to head (a03ab12).
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5705       +/-   ##
===========================================
+ Coverage    9.49%   26.04%   +16.55%     
===========================================
  Files          30       30               
  Lines        2634     2634               
===========================================
+ Hits          250      686      +436     
+ Misses       2362     1903      -459     
- Partials       22       45       +23     
Flag Coverage Δ
inttests 9.49% <ø> (ø)
unittests 18.90% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@manuelbuil manuelbuil merged commit 3d833e0 into rancher:master Apr 12, 2024
5 checks passed
@manuelbuil manuelbuil deleted the checkReservedVIP branch April 12, 2024 07:29
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.

4 participants