-
Notifications
You must be signed in to change notification settings - Fork 104
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
provider: added k3d provider and node lifecycle handlers #441
base: main
Are you sure you want to change the base?
provider: added k3d provider and node lifecycle handlers #441
Conversation
0c3a6e3
to
d25b6ab
Compare
/cc @cpanato |
|
/hold Holding this under the |
3b76b81
to
a70ae18
Compare
p := commandRunner.NewProc(installCommand) | ||
p.SetStdout(&stdout) | ||
p.SetStderr(&stderr) | ||
result := p.Run() | ||
if result.Err() != nil { | ||
return "", fmt.Errorf("failed to install %s: %s: \n %s", pPath, result.Result(), stderr.String()) |
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.
Adding these to be able to capture and log the install failures better during the provider install.
/unhold |
a70ae18
to
afe220a
Compare
46696b8
to
6a755b5
Compare
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.
/lgtm
thanks
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, harshanarayana 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 |
/hold |
Holding this until @cpanato @vladimirvivien @ShwethaKumbla also takes look |
/retest |
6a755b5
to
4c0f410
Compare
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.
/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.
@harshanarayana this a great (massive) PR.
Thanks for doing this and I am sure the community will appreciate the k3d integration.
I left some comments and we can work on getting this to the finish line.
support/types.go
Outdated
// test out how the Remove operation impacts your workflow. | ||
// Or you want to simulate a case where one or more node of your cluster is down and you want to see how | ||
// your application reacts to such failure events. | ||
type E2EClusterProviderWithImageLoaderAndNodeLifecycle interface { |
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.
Oof, that is a long name.
Let's reconsider making it simpler or break into multiple (embedded) interfaces.
E2EClusterProvier
E2EClusterProviderWithImgLoader
*E2EClusterProviderWithLifeCycle
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.
Ack. using E2EClusterProviderWithLifeCycle
instead
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.
@vladimirvivien Do you want to check the image loader related interface name too ?
4c0f410
to
3a6934c
Compare
New changes are detected. LGTM label has been removed. |
@vladimirvivien PTAL. I has pushed the requested changes. |
3a6934c
to
1597cf5
Compare
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.
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.
Making progress @harshanarayana
Added some more comments.
} | ||
log.V(4).InfoS("Stopping node in k3d cluster", "command", cmd) | ||
p, stdout, stderr := utils.FetchSeperatedCommandOutput(cmd) | ||
if p.Err() != nil || (p.Exited() && p.ExitCode() != 0) { |
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.
Oh boy, you are pushing that package (gexe) to it's limit. Love it 😉
|
||
// PerformNodeLifecycleOperation performs a node operation on a cluster. These operations can range from Add/Remove/Start/Stop. | ||
// This helper is re-used in both node lifecycle handler used as types.StepFunc or env.Func | ||
func PerformNodeLifecycleOperation(ctx context.Context, action support.NodeOperation, node *support.Node, args ...string) error { |
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.
Does this function only applies when using K3d or do you think orther providers will use that.
If only k3d, maybe it belongs in third_party package path if that is the case.
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.
Other providers like minikube can support this.. And kwok
technically can be simulated to enable node add and remove workflows. So it can work with other providers as well. We can do that even for kind
. But we will have to use adhoc docker command to simulate power on and off kind of workflows
This change includes the following changes and features. 1. Added a new Interface type `E2EClusterProviderWithLifeCycle` which can be used to setup providers that extend the cluster lifecycle function around the nodes. 2. Enabled a `k3d` based provider with support for Node lifecycle management. 3. Existing Image loader related function and interfaces were augmented with `args ...string` to be able to provide additional arguments in case if the image load handlers need some of the additional config.
1597cf5
to
3d1be3f
Compare
@vladimirvivien PTAL when you can. Addressed the required changes in the message for wait for control plane. |
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.
More comments.
@@ -0,0 +1,399 @@ | |||
/* |
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.
This should be in third-party.
I see that kwok in that package, but it should have also been in third-party.
package support is a place for API definitions for third-party support.
kind
was supposed to be an example implementation for others (maybe it should have been in third-party as well)
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.
Good point.. I can move them all. But we have to retain some sort of aliasing to aid the import backward compatibility.. At least for kwok and the kind that we support today in an available release.. Right?
If you are ok with that, I will take care of the required package refactoring
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.
Hi @harshanarayana if type aliasing will help maintain backward compat, then do it.
At the very least, we should just move kwok. Thanks for doing it.
p = commandRunner.RunProc("ls $GOPATH/bin") | ||
if p.Err() != nil { | ||
return "", fmt.Errorf("failed to install %s: %s", pPath, p.Err()) | ||
p = commandRunner.NewProc("ls $GOPATH/bin") |
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.
Do you think gexe
could be improved to make output caputure easier ? If so, please open an issue.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
@harshanarayana is this still on your radar for 2025? |
I will get this done before we cut the christmas release.. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR enables a new
E2EClusterProvider
fork3d
based infra.E2EClusterProviderWithImageLoaderAndNodeLifecycle
which can be used to setup providers that extend the cluster lifecycle function around the nodes.k3d
based provider with support for Node lifecycle management.args ...string
to be able to provide additional arguments in case if the image load handlers need some of the additional config.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I have also extended the
E2EClusterProviderWithImageLoader
's Image load related methods to take an additionalarg ...string
value to account for additional options that one can pass while performing image load operation alone.This comes in handy with
k3d
where it provides a few different mode of loading images and cleanup workflows.This change also doesn't break any existing API contract extended by the
envfuncs
.Does this PR introduce a user-facing change?
Additional documentation e.g., Usage docs, etc.: