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

Print selective deployment error to container stdio #229

Merged

Conversation

kate-goldenring
Copy link
Collaborator

We currently do not have a way to surface runwasi errors to K8s events. They are only being surfaced in containerd logs which are not intuitive to search. Just as we print trigger information on startup, it makes sense to print error reasons for the time being to make them easier to discover. Take an app with components hello and goodbye. If i selectively deploy component foo, I will now see the following log in the container log:

Error with selective deployment: Specified component "foo" not found in application

Example SpinApp:

apiVersion: core.spinkube.dev/v1alpha1
kind: SpinApp
metadata:
  name: hello-salutation-spinap-foop
spec:
  # TODO: change to image hosted at ghcr.io/spinkube/spin-operator/salutations when published
  image: "ghcr.io/kate-goldenring/spin-operator/examples/spin-salutations:20241022-144454"
  replicas: 1
  executor: containerd-shim-spin
  components: ["foo"]

ref #228

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

Given there is no easy way for a user to learn about this kind of issue (since we can't do k8s events), this seems a more than reasonable approach.

LGTM

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

For expedience sake, lgtm, but I'd like to see us provide a more precise way of logging messages to either pod or containerd log. Relying on println and stdout to be the right log destination concerns me.

Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

I agree this might be the best we can do right now but I'd like to see a comment in line 149 saying why we are relying on println.

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

+1 to everyone's comments. I believe this has to do with the way (and timing) of how we fork the container. There is some other discussion in runwasi to revisit the approach which may help.

@jsturtevant jsturtevant merged commit 9d892a5 into spinkube:main Nov 8, 2024
8 checks passed
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.

5 participants