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

Add system-info callback #15

Merged

Conversation

gabriel-samfira
Copy link
Contributor

A new callback is available in garm which allows runners to send info about themselves while installing. This new callback allows instances to report their os_name, os_version and the GH agent_id.

See cloudbase/garm#200

This change also makes sure that curl is installed in the image, otherwise the entrypoint will fail.

@gabriel-samfira gabriel-samfira marked this pull request as ready for review January 4, 2024 15:58
@gabriel-samfira gabriel-samfira force-pushed the enable-system-info-callback branch 3 times, most recently from 2745982 to e770fb0 Compare January 4, 2024 16:14
Comment on lines +27 to +30
OS_NAME=${NAME:-""}
OS_VERSION=${VERSION_ID:-""}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thinking about if we could set some meaningful default values for $NAME and $VERSION_ID maybe during build as ENV in the Dockerfile. Then a user could still overwrite these in their podTemplateSpec in the provdider config if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this stuff is strictly informational. It's nice to have, but not strictly necessary in terms of functionality. Simply put, it has no baring on functionality, but if a user does a:

garm-cli runner show <runner ID>

that command will show the real OS and version, not something we infer from the image itself.

This was always the way I hoped this information would be sent upstream (runners would send that info as part of their installation process), but only now got around to doing it.

The idea is that by default, it should be fine to have an empty string for this stuff. But if we can safely determine the OS and version , we should send that to garm.

The /etc/os-release file has been available for a very long time. So I expect it to be present in any container image that is based of any OS that;s not scratch.

@rafalgalaw
Copy link
Collaborator

Thanks for the contribution @gabriel-samfira! Would it make sense to add this callback to the summerwind image as well and if so could you add it there too? :)

@gabriel-samfira
Copy link
Contributor Author

Thanks for the contribution @gabriel-samfira! Would it make sense to add this callback to the summerwind image as well and if so could you add it there too? :)

not sure. Will look at that image as well. We sould theoretically do it there as well before we delegate the rest of the entrypoint execution to the default one. Will have a look later this week (traveling ATM)

A new callback is available in garm which allows runners to send info
about themselves from userdata. This new callback allows instances to
report their os_name, os_version and the GH agent_id.

See cloudbase/garm#200

This change allso makes sure that curl is installed in the image,
otherwise the userdata will never work.

Signed-off-by: Gabriel Adrian Samfira <[email protected]>
@gabriel-samfira gabriel-samfira force-pushed the enable-system-info-callback branch from e770fb0 to 36c6ea1 Compare January 13, 2024 19:44
@gabriel-samfira
Copy link
Contributor Author

gabriel-samfira commented Jan 13, 2024

sorry for the delay on this. I added the system info code to the summerwind image as well. It now shows up as "Ubuntu 22.04".

Screenshot from 2024-01-13 21-47-48

@rafalgalaw rafalgalaw self-requested a review January 17, 2024 14:43
Copy link
Collaborator

@rafalgalaw rafalgalaw left a comment

Choose a reason for hiding this comment

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

Also verified it locally and it looks good to me! Thank you very much and also sorry for my late response on this topic.

@rafalgalaw rafalgalaw merged commit 326ab30 into mercedes-benz:main Jan 17, 2024
2 checks passed
@gabriel-samfira gabriel-samfira deleted the enable-system-info-callback branch February 6, 2024 09:41
@gabriel-samfira gabriel-samfira restored the enable-system-info-callback branch February 6, 2024 09:41
@gabriel-samfira gabriel-samfira deleted the enable-system-info-callback branch February 6, 2024 09:41
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.

2 participants