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

kind.sh on macos including calico #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

KRANTHI0918
Copy link
Contributor

Updated the scripts to cover kubeslice kind cluster setup on mac os versions along Calico Installation.

@KRANTHI0918 KRANTHI0918 requested a review from a user June 21, 2022 16:55
@KRANTHI0918 KRANTHI0918 requested a review from pnavali as a code owner June 21, 2022 16:55
kubectl get pods -n calico-system
echo "Wait for Calico to be Running"
namespace=calico-system
sleep=900
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sleep 900 seems excessive? If it's not going to work, could we fail faster? Fwiw, in my testing it tool 32sec for calico to be ready... so 60 or 90 might be fine?

Copy link
Contributor Author

@KRANTHI0918 KRANTHI0918 Jun 21, 2022

Choose a reason for hiding this comment

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

It's taking more than 600secs in Amazon EC2, macOS Monterey 12.4 Instance, that why updated with Sleep 900.

done
done
done
}
Copy link
Collaborator

@eric978 eric978 Jun 21, 2022

Choose a reason for hiding this comment

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

I'm tempted to say wait_for_... should be generalized to wait for -condition- on -command- and be put into the util dir so anything can use it.

But we've got big changes in this file coming when Deepankar's slicectl is ready for use... so fine to hold off until then.

@@ -0,0 +1,425 @@
#!/usr/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we have to make a completely separate copy for mac? Was there no way to conditionalize the mac parts inside the script? It'll be a hassle maintaining two separate scripts and keeping them functionally in sync... would be better if there was just one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the high number of modifications in the script, We decided to make a separate copy for mac. There is a way to conditionalize the mac parts inside the script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

First, it doesn't look like an especially high number of mods. I see 14 total changes. Running macos-kind.sh on Ubuntu generally works (except for the check for colima... which I don't see used by the script isn't obviously used to me?). For each change:

  • getopts change works on linux
  • if check after getops is ok on linux
  • assuming the colima check is needed, set the OS to "mac" and make it conditional
  • The various "sleeps" should ideally be replaced by "wait_for_..." but if you want to leave them just pick a number that is good for both
  • Looks like the sed issue is just about:
    sed -i '' ...
    ...vs...
    sed ...
    So why not do something like:
    (at the top) BSDSED=" "
    (inside the macos plaform check make it two signle quotes inside the double quotes) BSDSED="''"
    Then make each sed line be:
    sed -i $BSDSED ...

Not sure what the "14i" sed for 14i is for? Can we fix that to work for both?

@kmjayadeep
Copy link
Contributor

looks like colima https://github.com/abiosoft/colima can create kubernetes cluster itself. Why do we need kind clusters then?

@eric978
Copy link
Collaborator

eric978 commented Jun 21, 2022 via email

@eric978
Copy link
Collaborator

eric978 commented Jul 29, 2022

@KRANTHI0918 What's going on with this PR? Were you going to make the changes?

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