Skip to content

[preview] Fix deploy of resources in multiple ns #16924

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

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Conversation

WVerlaek
Copy link
Member

@WVerlaek WVerlaek commented Mar 20, 2023

Description

Fixes resources that exist in multiple namespaces under the same name (e.g. ws-manager-mk2's role) only getting deployed in one namespace

Related Issue(s)

How to test

Create preview env

Release Notes

NONE

Documentation

Build Options:

  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-wv-fix-deploy-gitpod-ns.1 because the annotations in the pull request description changed
(with .werft/ from main)

@@ -617,7 +617,7 @@ rm -f "${INSTALLER_RENDER_PATH}"
# Wait for objects to be ready
# =========================
for item in deployment.apps/blobserve deployment.apps/content-service deployment.apps/dashboard deployment.apps/ide-metrics deployment.apps/ide-proxy deployment.apps/ide-service deployment.apps/image-builder-mk3 deployment.apps/minio deployment.apps/node-labeler deployment.apps/payment-endpoint deployment.apps/proxy deployment.apps/public-api-server deployment.apps/redis deployment.apps/server deployment.apps/spicedb deployment.apps/usage deployment.apps/ws-manager deployment.apps/ws-manager-bridge deployment.apps/ws-proxy statefulset.apps/messagebus statefulset.apps/mysql statefulset.apps/openvsx-proxy daemonset.apps/agent-smith daemonset.apps/fluent-bit daemonset.apps/registry-facade daemonset.apps/ws-daemon; do
kubectl --kubeconfig "${PREVIEW_K3S_KUBE_PATH}" --context "${PREVIEW_K3S_KUBE_CONTEXT}" rollout status "${item}"
kubectl --kubeconfig "${PREVIEW_K3S_KUBE_PATH}" --context "${PREVIEW_K3S_KUBE_CONTEXT}" --namespace "${PREVIEW_NAMESPACE}" rollout status "${item}"
Copy link
Member Author

Choose a reason for hiding this comment

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

was seeing not-found errors here, setting the namespace fixed that. Think the namespace gets updated and doesn't point to default at this point?

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 same change was also made here, to fix triggering workspace integration tests in werft.

@vulkoingim @mads-hartmann can we trouble you for reviews?

@@ -604,7 +604,7 @@ export -f diff-apply
mkdir temp-installer || true
pushd temp-installer
# this will split the big yaml produced by the installer, so we can diff individual parts of it and run them in parallel
yq4 -s '.kind + "_" + .metadata.name' "../${INSTALLER_RENDER_PATH}"
yq4 -s '.kind + "_" + .metadata.namespace // "" + "_" + .metadata.name' "../${INSTALLER_RENDER_PATH}"
Copy link
Member Author

@WVerlaek WVerlaek Mar 20, 2023

Choose a reason for hiding this comment

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

Pushed a fix so that // "" defaults to an empty string if the namespace field isn't set (e.g. for namespaces 🙃), otherwise those resources would get skipped

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-wv-fix-deploy-gitpod-ns.4 because the annotations in the pull request description changed
(with .werft/ from main)

@kylos101
Copy link
Contributor

@kylos101
Copy link
Contributor

kylos101 commented Mar 21, 2023

/hold

doesn't work for fresh installs, I tested liked so:

leeway run dev/preview:delete-preview
leeway run dev:preview

@kylos101
Copy link
Contributor

kylos101 commented Mar 21, 2023

/unhold

Limited the default namespace check to the namespace field (or lack there of)..

Without this change, the yaml split yaml file names were just kind and namespace, so they were overwriting each other, and Gitpod wasn't being installed.

Test:
image

@kylos101
Copy link
Contributor

kylos101 commented Mar 21, 2023

Question, non-blocking:
@WVerlaek what is the expectation of the installer option with-ws-manager-mk2? It does not appear to work, but maybe I was impatient or did something wrong? I saw mk1 after using. 🤔

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-wv-fix-deploy-gitpod-ns.6 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-wv-fix-deploy-gitpod-ns.7 because the annotations in the pull request description changed
(with .werft/ from main)

@WVerlaek
Copy link
Member Author

WVerlaek commented Mar 21, 2023

Without this change, the yaml split yaml file names were just kind and namespace, so they were overwriting each other, and Gitpod wasn't being installed.

🤦 thanks

@WVerlaek
Copy link
Member Author

WVerlaek commented Mar 21, 2023

what is the expectation of the installer option with-ws-manager-mk2? It does not appear to work, but maybe I was impatient or did something wrong? I saw mk1 after using. 🤔

haven't used that option, but think it would have to be with-wsman-mk2, at least that was the werft annotation to use. For running locally you can do GITPOD_WSMANAGER_MK2=true leeway run dev:preview

@WVerlaek WVerlaek force-pushed the wv/fix-deploy-gitpod-ns branch from 1f40216 to 87633be Compare March 21, 2023 09:23
@roboquat roboquat merged commit 8260531 into main Mar 21, 2023
@roboquat roboquat deleted the wv/fix-deploy-gitpod-ns branch March 21, 2023 09:30
@WVerlaek
Copy link
Member Author

Thanks for the review @mads-hartmann , I rebased and it merged the PR even though creating the preview env failed (too many certs issued), is that expected?

Does seem to work on main now, created a preview env successfully

@mads-hartmann
Copy link
Contributor

mads-hartmann commented Mar 21, 2023

@WVerlaek Ah, I think that's probably because we don't currently have the "create preview"-related jobs marked as required - @vulkoingim might have more context around if this is intended or not ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants