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

[RFC-13] Refactor run.ps1 #60

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

HarrisonWAffel
Copy link
Contributor

@HarrisonWAffel HarrisonWAffel commented Oct 10, 2024

Issues: rancher/rancher#45386, rancher/rancher#46344, rancher/rancher#45385

This PR refactors run.ps1, many of the build scripts used to create development images of the system-agent-installer-rke2 on Windows, and makes minor changes to the CI workflows.

The core changes are located in run.ps1, and cover the following

  • run.ps1 will now compare the value of the $env:INSTALL_RKE2_VERSION environment variable and the /usr/local/bin/rke2.exe --version output to determine if the node is undergoing an upgrade. If the environment variable or the output of rke2.exe --version are are empty, or if they do not match, then rke2 will be stopped if needed and reinstalled onto the node. Properly providing INSTALL_RKE2_VERSION will prevent rke2 from being stopped and reinstalled unnecessarily. Going forward, this variable will always be provided by the Rancher planner
  • the rke2 service will always be configured with a Delayed start type. This ensures that rke2 starts after other, potentially more disruptive, services have started
  • The restart stamp file is now properly handled and updated in accordance with the WINS_RESTART_STAMP environment variable provided by the Rancher planner. Additionally, the $RESTART variable is properly utilized to restart the rke2 service only when needed.
  • Handling of rke2 environment variables has been updated to properly remove environment variables from the registry if they are no longer provided

The changes to powershell files such as make.ps1 and build.ps1 do not impact CI and are only included to make local development easier.

@HarrisonWAffel HarrisonWAffel requested a review from a team October 10, 2024 22:22
@HarrisonWAffel HarrisonWAffel requested a review from a team as a code owner October 10, 2024 22:22
@brandond
Copy link
Member

brandond commented Oct 10, 2024

compare the value of the $env:INSTALL_RKE2_VERSION environment variable and the /var/lib/rancher/agent/images/runtime-image.txt file to determine if the node is undergoing an upgrade.

I take it you mean, compare the image tag from the file to the version, after replacing the + in the version with -?

Also I don't believe we have any specific contract around that file existing or containing only a single entry; I'm hesitant to agree to using this as an indicator of the currently installed RKE2 version. The k3s-upgrade image used by SUC actually just runs k3s -v: k3s-io/k3s-upgrade#66

We never got around to adding this same behavior to rke2-upgrade, but I'd prefer to actually check the version reported by the currently installed binary, instead of parsing tags from text files that were not meant to be used as an installed version indicator.

@HarrisonWAffel
Copy link
Contributor Author

HarrisonWAffel commented Oct 11, 2024

The k3s-upgrade image used by SUC actually just runs k3s version: k3s-io/k3s-upgrade#66

Good to know! I had thought about using --version when I was initially drafting the RFC but was concerned that there was no guarantee about the output format , whereas the runtime-image.txt file content would always be the same. If there's prior art in k3s which also uses --version output for a similar purpose them I'm okay with changing run.ps1 to do the same for rke2. I've updated the Determine-Upgrading function to use rke2.exe --version

Copy link
Contributor

@jakefhyde jakefhyde left a comment

Choose a reason for hiding this comment

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

Minor nits

package/run.ps1 Show resolved Hide resolved
$VERSION = $VERSION.Replace('+', '-')

#export stuff out
$env:VERSION = $VERSION
$env:COMMIT = $COMMIT
$env:REPO = "rancher"
$env:IMAGE = "$REPO/system-agent-installer-rke2:$VERSION"
$env:IMAGE = "$env:REPO/system-agent-installer-rke2:$VERSION"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can $env:REPO ever be unset? Maybe use the `REPO = if () {} else {""}; paradigm instead

Copy link
Member

@brandond brandond Oct 15, 2024

Choose a reason for hiding this comment

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

It is occasionally overridden by devs if they are trying to build a custom image, but even in that case people usually just retag it before pushing it. I suppose we could also see more use of this as we start building and pushing directly to Prime? It should never be empty.

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've reverted a few of the changes I've made to this file and the download.ps1 file just to be super sure this won't negatively impact CI. After looking closer I was able to fix the issues I was having when building locally with fewer changes. The CI right now only relies on download.ps1 which relies on version.ps1.

@HarrisonWAffel HarrisonWAffel merged commit b5a25f2 into rancher:main Oct 18, 2024
4 checks passed
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.

3 participants