Skip to content

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

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 spinframework: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