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

k3s component actually waits for node to come up #366

Merged
merged 2 commits into from
Mar 8, 2022

Conversation

YrrepNoj
Copy link
Contributor

@YrrepNoj YrrepNoj commented Mar 7, 2022

The previous command was just running the 'k get nodes' command without verifying any nodes were actually returned.

Description

As part of the 'after' script definitions for the k3s component we will now verify that at least 1 k3s node is returned when 'kubectl get nodes' is executed.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

The previuos command was just running the 'k get nodes' command without
verifying any nodes were actually returned.
@YrrepNoj YrrepNoj requested a review from jeff-mccoy March 7, 2022 20:02
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Mar 7, 2022

I was originally doing a k ... -o json and then using jq to parse the items list to make sure the list had more than one item but since 'jq' isn't default on some OS's I ended up doing more simple grep.

@RothAndrew
Copy link
Contributor

I think the reason that kubectl get nodes works as is is that it returns a non-zero exit code if it can't connect to the cluster. We don't actually care how many nodes there are, just that we are making a good connection to the cluster.

@jeff-mccoy
Copy link
Contributor

I think we may need to confirm wc/ Grep actually exist on our target OS's as well--or do it in code and get rid of this step. I actually think something that blocks everything in code until there is a complete node is cleaner than this anyway. Thoughts?

@RothAndrew
Copy link
Contributor

Maybe something like this?

components:
  - name: k3s
    needsK8s: false
    ...

  - name: InstallSomeFile
    needsK8s: false
    ...

  - name: someFooBarK8sDeployment
    needsK8s: true
    ...

@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Mar 7, 2022

I think the reason that kubectl get nodes works as is is that it returns a non-zero exit code if it can't connect to the cluster. We don't actually care how many nodes there are, just that we are making a good connection to the cluster.

It will not return an error code if the nodes are not up yet, which is a race condition we ran into when trying to get the node architecture when initializing with k3s. We care that at least 1 node is there.

@RothAndrew
Copy link
Contributor

That makes sense.

If this very small change fixes that than I'm all for it. Something bigger that resolves the issue in a more elegant way can come later

@jeff-mccoy
Copy link
Contributor

Maybe something like this?

components:

  - name: k3s

    needsK8s: false

    ...



  - name: InstallSomeFile

    needsK8s: false

    ...



  - name: someFooBarK8sDeployment

    needsK8s: true

    ...

You don't even need that. The only time we don't need k8s is when you are only moving files and that is already accounted for in the package deploy stage. We already know if you need k8s. I think rather than using scripts (which was a band-aid and brittle) we should just either add something to the k8s getstate function or some call before that to make sure we have a real/valid k8s connection to a running cluster.

@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Mar 7, 2022

I'll replace this PR with something like that then :)

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

@YrrepNoj I believe you're going to rewrite this to use something in go instead

@YrrepNoj YrrepNoj force-pushed the verify-k3s-node-exists branch from fbe0f56 to 045f3ba Compare March 8, 2022 05:36
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Mar 8, 2022

@jeff-mccoy I think this should be more in line with what you were thinking

@jeff-mccoy
Copy link
Contributor

I think there's still some issue with the test--but this is good enough to merge and finish cleaning in the main branch now. Thanks!

@jeff-mccoy jeff-mccoy merged commit e2ea728 into feature/injection-support Mar 8, 2022
@jeff-mccoy jeff-mccoy deleted the verify-k3s-node-exists branch March 8, 2022 07:33
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