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

update windows uninstall and install scripts #1872

Merged
merged 1 commit into from Oct 8, 2021

Conversation

rosskirkpat
Copy link
Contributor

@rosskirkpat rosskirkpat commented Sep 23, 2021

Proposed Changes

Update Windows uninstall and install scripts:

  • will now clean env vars (including machine env vars) on uninstall
  • ctr commands should now successfully complete if it is present
  • fix a few typos throughout both scripts
  • will now clean custom directories for CATTLE_AGENT_BIN_PREFIX, CATTLE_AGENT_VAR_DIR, CATTLE_AGENT_CONFIG_DIR during uninstall
  • added retries to the query of the v3/connect/config-yaml endpoint as the content can take longer than expected to generate
  • added additional INFO outputs to better understand what succeeds when the script runs
  • enable Strict mode in all scripts
  • fix default directory for CATTLE_AGENT_VAR_DIR now defaults to C:/var/lib/rancher/agent instead of C:/etc/rancher/agent
  • added CATTLE_AGENT_BIN_PREFIX env var as it exist in system agent install script. defaults to C:/usr/local
  • declare CATTLE_AGENT_VAR_DIR, CATTLE_AGENT_CONFIG_DIR , CATTLE_AGENT_BIN_PREFIX as machine env vars
  • fix naming issue when looking for airgap tarballs for commit testing

Docs should be updated to reflect a few new env vars in the install script and what they do/how to use them

Types of Changes

bug fixes and expanding the functionality of the install and uninstall scripts

Verification

the updated scripts were used on Windows rke2 agent nodes in custom clusters to verify they worked.

Linked Issues

#1873
#1890

@rosskirkpat rosskirkpat added this to the v1.22.3+rke2r1 milestone Sep 23, 2021
@rosskirkpat rosskirkpat self-assigned this Sep 23, 2021
@rosskirkpat rosskirkpat requested a review from a team as a code owner September 23, 2021 16:30
@rosskirkpat rosskirkpat added this to Working in Development [DEPRECATED] via automation Sep 23, 2021
Development [DEPRECATED] automation moved this from Working to Approved PRs Sep 23, 2021
@rosskirkpat rosskirkpat moved this from Approved PRs to Peer Review in Development [DEPRECATED] Sep 23, 2021
Development [DEPRECATED] automation moved this from Peer Review to Approved PRs Sep 23, 2021
@rosskirkpat
Copy link
Contributor Author

I have a few more testing scenarios that I would like to run through prior to merging but every test that I have run so far has the updated scripts working as expected.

Copy link
Contributor

@rancher-max rancher-max left a comment

Choose a reason for hiding this comment

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

There's at least two issues with installing via commit:

  1. Random bracket that shouldn't exist on line 163 in install.ps1
  2. Something wrong with checksums.
    See error when using this install script:
PS C:\Users\Administrator> ./install.ps1 -Commit 9517e166151f202814d5a8856ab038ec53f43d4a
[INFO] Windows feature: 'Containers' is installed. Installation will proceed.
Using stable channel of rke2 for installation
[INFO] using commit 9517e166151f202814d5a8856ab038ec53f43d4a} as release
Get-AirgapChecksums : A parameter cannot be found that matches parameter name 'Uri'.
At C:\Users\Administrator\install.ps1:567 char:41
+ ... _EXPECTED = Get-AirgapChecksums -CommitHash $Commit -StorageUrl $STOR ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (:) [Get-AirgapChecksums], ParameterBindingException
    + FullyQualifiedErrorId : NamedParameterNotFound,Get-AirgapChecksums 

Development [DEPRECATED] automation moved this from Approved PRs to Peer Review Sep 28, 2021
@rosskirkpat
Copy link
Contributor Author

@rancher-max the errant bracket has been removed and this should also fix the checksum issue as the commitHash was being passed with the }. Would you be able to re-test?

Copy link
Contributor

@phillipsj phillipsj left a comment

Choose a reason for hiding this comment

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

LGTM!

@rancher-max
Copy link
Contributor

@rancher-max the errant bracket has been removed and this should also fix the checksum issue as the commitHash was being passed with the }. Would you be able to re-test?

I gave it a try, but still get the checksum error 🤔

 ./install.ps1 -Commit 9517e166151f202814d5a8856ab038ec53f43d4a
[INFO] Windows feature: 'Containers' is installed. Installation will proceed.
Using stable channel of rke2 for installation
[INFO] using commit 9517e166151f202814d5a8856ab038ec53f43d4a as release
Get-AirgapChecksums : A parameter cannot be found that matches parameter name 'Uri'.
At C:\Users\Administrator\install.ps1:567 char:41
+ ... _EXPECTED = Get-AirgapChecksums -CommitHash $Commit -StorageUrl $STOR ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (:) [Get-AirgapChecksums], ParameterBindingException
    + FullyQualifiedErrorId : NamedParameterNotFound,Get-AirgapChecksums

Looking into the parameters, I found one passing Uri on line 421:

    if (!(Test-Download -Url $AirgapChecksumsUrl)) {
        $AirgapChecksumsUrl = "$StorageUrl/rke2-images.$suffix$CommitHash.tar.gz.sha256sum"
    }

Changing that to Url resolves the error, but leads to a new one:
Get-AirgapChecksums : A parameter cannot be found that matches parameter name 'Path'.

This seems to occur on line 426: return Find-Checksum -Path $TempAirgapChecksums -Pattern "rke2-images.$suffix.tar" which I updated Path to ChecksumFilePath.

This then appears to work without compilation errors, but gives an error at the end:

PS C:\Users\Administrator> ./install.ps1 -Commit 9517e166151f202814d5a8856ab038ec53f43d4a
[INFO] Windows feature: 'Containers' is installed. Installation will proceed.
Using stable channel of rke2 for installation
[INFO] using commit 9517e166151f202814d5a8856ab038ec53f43d4a as release
[INFO] downloading airgap checksums at https://storage.googleapis.com/rke2-ci-builds/rke2-images.windows-amd649517e166151f202814d5a8856ab038ec53f43d4a.tar.zst.sha256sum
[ERROR] Checksum file wasn't found: C:\Users\ADMINI~1\AppData\Local\Temp\2\rke2-install\rke2-images.checksums

@rosskirkpat

Copy link
Contributor

@phillipsj phillipsj left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@phillipsj phillipsj left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #1872 (943d6cc) into master (2f6048e) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1872      +/-   ##
=========================================
+ Coverage    8.58%   8.59%   +0.01%     
=========================================
  Files          21      21              
  Lines        1887    1884       -3     
=========================================
  Hits          162     162              
+ Misses       1698    1695       -3     
  Partials       27      27              
Flag Coverage Δ
unittests 8.59% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
pkg/cli/cmds/root.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f6048e...943d6cc. Read the comment docs.

@rosskirkpat rosskirkpat changed the title [WIP] update windows uninstall and install scripts update windows uninstall and install scripts Oct 8, 2021
bundle/bin/rke2-uninstall.ps1 Outdated Show resolved Hide resolved
bundle/bin/rke2-uninstall.ps1 Show resolved Hide resolved
@rosskirkpat rosskirkpat force-pushed the feature/windows-script-fixes branch 2 times, most recently from 2ddbe08 to 136430f Compare October 8, 2021 17:27
Copy link
Contributor

@phillipsj phillipsj left a comment

Choose a reason for hiding this comment

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

LGTM

@phillipsj phillipsj merged commit 9e99d4b into rancher:master Oct 8, 2021
Development [DEPRECATED] automation moved this from Peer Review to Done Issue / Merged PR Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development [DEPRECATED]
Done Issue / Merged PR
Development

Successfully merging this pull request may close these issues.

None yet

5 participants