-
Notifications
You must be signed in to change notification settings - Fork 89
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
Convert Upgrader Script to Go binary #3033
Convert Upgrader Script to Go binary #3033
Conversation
Skipping CI for Draft Pull Request. |
acc26a0
to
27046ed
Compare
/hold |
da55830
to
a4dbffc
Compare
/retest |
1 similar comment
/retest |
projects/aws/upgrader/cmd/root.go
Outdated
var rootCmd = &cobra.Command{ | ||
Use: "upgrader", | ||
Short: "EKS Anywhere InPlace upgrader", | ||
Long: `Use EKS Anywhere InPlace upgrader to upgrade your nodes InPlace`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long: `Use EKS Anywhere InPlace upgrader to upgrade your nodes InPlace`, | |
Long: `Use EKS Anywhere InPlace upgrader to upgrade your nodes in place`, |
func upgradeCniPlugins(ctx context.Context) error { | ||
upg := upgrade.NewUpgrader() | ||
if err := upg.CniPluginsUpgrade(ctx); err != nil { | ||
return fmt.Errorf("upgrading Cni-Plugins on the node: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("upgrading Cni-Plugins on the node: %v", err) | |
return fmt.Errorf("upgrading cni-plugins on the node: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should have code comments on all exported functions. I highly suggest you add more code comments around why things are being done the way they are, why code exists, etc.
"path/filepath" | ||
"strings" | ||
|
||
"github.com/aws/eks-anywhere-build-tooling/tools/version-tracker/pkg/util/logger" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What value does this library provide over just using the logr
package directly? Also, this package has not avoided package state in its implementation. There is no reason for a logger to have package state, it's a solved problem. I recommend not using it and to make all dependencies explicit by passing them into funds and methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reviewed the makefile changes. look pretty decent!
var upgradeContainerdCmd = &cobra.Command{ | ||
Use: "containerd", | ||
Short: "Upgrade containerd", | ||
Long: "Use InPlace Upgrader upgrade containerd to upgrade containerd on the node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long: "Use InPlace Upgrader upgrade containerd to upgrade containerd on the node", | |
Long: "Use upgrade containerd command to upgrade containerd on the node", |
|
||
upgCmpDir, err := u.upgradeComponentsDir() | ||
if err != nil { | ||
return fmt.Errorf("getting upgrade componenets directory: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("getting upgrade componenets directory: %v", err) | |
return fmt.Errorf("getting upgrade components directory: %v", err) |
func (u *Upgrader) KubeletKubectlUpgrade(ctx context.Context) error { | ||
cmpDir, err := u.upgradeComponentsKubernetesBinDir() | ||
if err != nil { | ||
return fmt.Errorf("getting upgrade componenets binary directory: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("getting upgrade componenets binary directory: %v", err) | |
return fmt.Errorf("getting upgrade components binary directory: %v", err) |
bd1a4aa
to
2ae08ce
Compare
/retest |
ee6b347
to
001c4cb
Compare
Signed-off-by: Rahul Ganesh <rahulgab@amazon.com>
cbe4b3e
to
8284678
Compare
Signed-off-by: Rahul Ganesh <rahulgab@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im good with the makefile changes, thanks for the updates!
Signed-off-by: Rahul Ganesh <rahulbabu95@gmail.com>
} | ||
|
||
cniVersionCmd := []string{"/opt/cni/bin/loopback", "--version"} | ||
out, err := u.ExecCommand(ctx, cniVersionCmd[0], cniVersionCmd[1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed and where is it being used?
} | ||
|
||
containerdVersionCmd := []string{"containerd", "--version"} | ||
out, err := u.ExecCommand(ctx, containerdVersionCmd[0], containerdVersionCmd[1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here. Let’s at least log the before version if we are calling it
Signed-off-by: Rahul Ganesh <rahulbabu95@gmail.com>
Signed-off-by: Rahul Ganesh <rahulbabu95@gmail.com>
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
/woof
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/unhold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhay-krishna, abhinavmpandey08, rahulbabu95 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 |
Issue #, if available:
Upgrader project consumed by eks-a for InPlace upgrades uses an upgrader script written in shell for running the upgrade commands on the node. Ideally, we need it as a Go binary for long term maintainability, testability and general conformance with other parts of the codebase.
Description of changes:
This change converts the upgrader shell script into a Go binary along with unit tests to cover the core business logic. Manually tested by copying over the Go binary to the nodes to test if the corresponding upgrade logic is working as expected when the binary is invoked instead of the shell script.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.