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

hack: add helper interactive script for setting peer-pods-cm #448

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

snir911
Copy link
Contributor

@snir911 snir911 commented Aug 22, 2024

run locally once OSC operator is installed and KUBECONFIG is set. currently supports AWS and Azure clusters

This script intends to be simple as possible ans still be able to set all defaults..

@snir911 snir911 self-assigned this Aug 22, 2024
@openshift-ci openshift-ci bot requested review from gkurz and pmores August 22, 2024 12:00
@snir911
Copy link
Contributor Author

snir911 commented Aug 29, 2024

@Saripalli-lavanya pls see if this workflow could help for the libvirt case as well (#436), thanks

hack/pp-cm-helper.sh Outdated Show resolved Hide resolved
hack/pp-cm-helper.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm
Tested it on my existing ARO setup by commenting out applyCM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2024
@bpradipt bpradipt requested a review from littlejawa September 25, 2024 09:24
AZURE_INSTANCE_SIZE=${AZURE_INSTANCE_SIZE:-Standard_B2als_v2}
AZURE_INSTANCE_SIZES=${AZURE_INSTANCE_SIZES:-Standard_B2als_v2,Standard_D2as_v5,Standard_D4as_v5,Standard_D2ads_v5}
[[ "${DISABLECVM}" == true ]] && AZURE_INSTANCE_SIZE=${AZURE_INSTANCE_SIZE:-Standard_B2als_v2} || AZURE_INSTANCE_SIZE=${AZURE_INSTANCE_SIZE:-Standard_DC2as_v5}
[[ "${DISABLECVM}" == true ]] && AZURE_INSTANCE_SIZES=${AZURE_INSTANCE_SIZES:-Standard_B2als_v2,Standard_D2as_v5,Standard_D4as_v5,Standard_D2ads_v5}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't define AZURE_INSTANCE_SIZES for CoCo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User may define it, i didn't have an idea of an alternative types to suggest, actually, there's a problem with CoCo which the instance types are not available across regions commonly as normal VMs, hence, it's better user to set it explicitly

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for start this should be ok..
Also @Saripalli-lavanya, can some of the libvirt cm config be added here to start with. It'll be easier and faster imho.

Copy link

@Saripalli-lavanya Saripalli-lavanya Oct 8, 2024

Choose a reason for hiding this comment

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

Sure, @bpradipt. For libvirt, the common optional variables are "CLOUD_PROVIDER", "PROXY_TIMEOUT", and "DISABLECVM". Additionally, LIBVIRT_IMAGE_ID would suffice. The rest are being handled through peer pod secrets.

@bpradipt
Copy link
Contributor

bpradipt commented Oct 8, 2024

/retest

hack/pp-cm-helper.sh Outdated Show resolved Hide resolved
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

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

@snir911 do you mind adding a md file or some headers to explain why this is important and when we should use it?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2024
Copy link
Contributor

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2024
${CLI} get cm peer-pods-cm -n openshift-sandboxed-containers-operator -o jsonpath='{.data}' | jq
if ${CLI} get ds/peerpodconfig-ctrl-caa-daemon -n openshift-sandboxed-containers-operator > /dev/null 2>&1; then
[[ -n $YES ]] || (read -r -p "Restart DeamonSet so that CM will be taken into account? [y/N] " && [[ "$REPLY" =~ ^[Yy]$ ]]) || exit 0
${CLI} set env ds/peerpodconfig-ctrl-caa-daemon -n openshift-sandboxed-containers-operator REBOOT="$(date)"

Choose a reason for hiding this comment

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

Hi @snir911 !

One suggestion of a future improvement: next, you can ask user if she/he want to wait for the daemonset restart.

hack/pp-cm-helper.sh Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2024
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

hack/pp-cm-helper.sh Show resolved Hide resolved
hack/pp-cm-helper.sh Outdated Show resolved Hide resolved
run locally once OSC operator is installed and KUBECONFIG is set.
currently supports AWS and Azure clusters
Copy link

openshift-ci bot commented Nov 19, 2024

@snir911: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@wainersm
Copy link

hi @snir911 ! Thanks that helper is really a helper :)
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2024
@snir911 snir911 merged commit 91bd600 into openshift:devel Nov 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants