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

Added check if the node is rebooted before the networks is deleted on windows #6437

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

rbrtbnfgl
Copy link
Contributor

@rbrtbnfgl rbrtbnfgl commented Jul 31, 2024

Proposed Changes

This PR move the delete of all networks on a script that does as Calico that checks if the node is rebooted only.

The fix right now works only if the other version has already the new script check updating from an older version requires a node reboot and the bug is fixed from the next version updates.

Types of Changes

Verification

Testing

  1. Run a pod on windows
  2. Update RKE2 version on the windows node
  3. The pod will have connectivity issue
  4. Update RKE2 from a fixed version to a new fixed version
  5. No issue should occur

Linked Issues

#5551

User-Facing Change


Further Comments

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.25%. Comparing base (69bc12d) to head (0b95c53).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6437      +/-   ##
==========================================
+ Coverage   25.23%   25.25%   +0.01%     
==========================================
  Files          33       33              
  Lines        2829     2831       +2     
==========================================
+ Hits          714      715       +1     
- Misses       2068     2070       +2     
+ Partials       47       46       -1     
Flag Coverage Δ
inttests 9.64% <ø> (-0.01%) ⬇️
unittests 17.80% <ø> (+0.02%) ⬆️

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.

@rbrtbnfgl rbrtbnfgl marked this pull request as ready for review August 1, 2024 07:14
@rbrtbnfgl rbrtbnfgl requested a review from a team as a code owner August 1, 2024 07:14
@@ -302,7 +302,8 @@ func (c *Calico) Start(ctx context.Context) error {

// generateCalicoNetworks creates the overlay networks for internode networking
func (c *Calico) generateCalicoNetworks() error {
if err := deleteAllNetworks(); err != nil {
cmd := exec.Command("powershell.exe", "C:\\var\\lib\\rancher\\rke2\\bin\\node-calico.ps1")
Copy link
Member

Choose a reason for hiding this comment

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

We have in the past pushed back on just having golang call out to powershell to do things. If we wanted to do this, we could just use upstream's powershell scripts in the first place, instead of handling cni setup in golang. Why do we need a powershell script to check a registry key and reinitialize hns?

Copy link
Contributor Author

@rbrtbnfgl rbrtbnfgl Aug 5, 2024

Choose a reason for hiding this comment

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

It needs to check the latest reboot. If Calico os starting after a reboot it has to clean the network if it's after a restart of RKE2 the network shouldn't be cleaned. I can try to do the check inside golang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@rbrtbnfgl rbrtbnfgl force-pushed the windows_fix branch 3 times, most recently from d08776a to 7af5676 Compare August 7, 2024 06:51
@rbrtbnfgl rbrtbnfgl changed the title Moved windows network delete on script Added check of the node is rebooted before the networks is deleted on windows Aug 7, 2024
func isNodeRebooted(ctx context.Context) (bool, error) {
path, err := exec.LookPath("powershell")
if err == nil {
cmd := exec.CommandContext(ctx, path, "-NoProfile", "-NonInteractive", "@(Get-CimInstance Win32_OperatingSystem).LastBootUpTime.ToString(\"G\")")
Copy link
Member

Choose a reason for hiding this comment

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

You're still execing powershell. You should be able to do this using pure go. See for example: https://gistlib.com/go/get-system-boot-time-in-milliseconds-in-go

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 was searching for something similar but it seems that syscall it's not working properly on windows. I tried that code and it's not building. I'll try to check if there is something similar.

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 did something similar using a different function. Now I'm not using any exec command so I also removed the context and left as it was before.

@rbrtbnfgl rbrtbnfgl changed the title Added check of the node is rebooted before the networks is deleted on windows Added check if the node is rebooted before the networks is deleted on windows Aug 9, 2024
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.

nit on storing time in the registry.

pkg/windows/utils.go Outdated Show resolved Hide resolved
@rbrtbnfgl rbrtbnfgl force-pushed the windows_fix branch 2 times, most recently from 8b5ec0d to 258b4db Compare August 12, 2024 14:48
Signed-off-by: Roberto Bonafiglia <roberto.bonafiglia@suse.com>
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.

5 participants