Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Add option to change ID prefix #757

Merged
merged 11 commits into from
Feb 1, 2021
Merged

Add option to change ID prefix #757

merged 11 commits into from
Feb 1, 2021

Conversation

bboreham
Copy link
Contributor

Prefixing everything with ignite- cuts into the number of characters available in some situations.

@stealthybox
Copy link
Contributor

I'm supportive of this change.

I'm curious which character limits were you running into and how many more you needed to make what you ran into work?
Hopefully other CNI's don't need shorter ID's or some kind of truncation...

Adding the global flag makes sense, but we should really get thing plumbed into the ignite config.
Would you be willing to do so?

The only tricky part is making the flag override happen in the proper order.
This should be pretty similar to the code-path for configuring the container/docker runtime via flag/config.

@bboreham
Copy link
Contributor Author

bboreham commented Jan 7, 2021

Weave Net creates a device named like vethweplfirst7c out of the first 7 characters of the ID you give it. Interface names are limited to 15 bytes.

The first 4 - veth - are necessary to coexist with standard Linux networking tools. We could tweak the next 4 - "we" is for Weave, "p" is for public and "l" is link, if I remember correctly, but even after that, taking the first 7 with ignite- really cuts into the diversity of names.

We could also re-code Weave Net to hash the name instead of taking the prefix, at the cost of making things harder to track down when you need to troubleshoot.

I don't know what other CNI implementations do in this situation.

@bboreham
Copy link
Contributor Author

I added namePrefix to the config.

(I added it to v1alpha3 at the same time; not sure if that is meant to be immutable but it will be backwards-compatible)

@stealthybox
Copy link
Contributor

We're working on making this work in ignite-spawn

@bboreham bboreham force-pushed the prefix-option branch 2 times, most recently from 98faede to 450f281 Compare January 18, 2021 17:10
@bboreham
Copy link
Contributor Author

I have rebased after the flaky test was fixed, and added a commit to pass the option through to ignite-spawn

@stealthybox
Copy link
Contributor

This patch is very close to being mergeable.

( nit: namePrefix,--name-prefix is misleading.
idPrefix,--id-prefix or systemPrefix is better in retrospect. )

We think a good addition would be patching the value into the VM Status on the first write of the VM object to disk.
This would be before the ignite-spawn handoff, so we wouldn't need the ignite-spawn flag to pass the value, since ignite-spawn has access to the VM object.

This allows us to remove the error-burden of flags for commands like ignite vm rm where the VM already exists and the value has to match.

This behavior would closely match the runtime/network status added in v0.8.0, and there's existing patching logic and tests that we can rely on.

Sunny and I plan to give this a more detailed look tomorrow.
Thanks for the continued work on getting this to work properly.

@stealthybox stealthybox force-pushed the prefix-option branch 3 times, most recently from f5b2851 to 9d68308 Compare February 1, 2021 07:58
bboreham and others added 7 commits February 1, 2021 00:58
Prefixing everything with `ignite-` cuts into the number of characters
available in some situations.
New field is called namePrefix

(I added it to v1alpha3 at the same time; not sure if that is meant
to be immutable but it will be backwards-compatible)
And set the default if not passed
@stealthybox stealthybox force-pushed the prefix-option branch 2 times, most recently from 73bd8d3 to 2e3af7b Compare February 1, 2021 16:21
Copy link
Contributor

@darkowlzz darkowlzz 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.

@darkowlzz darkowlzz merged commit fcf1faf into master Feb 1, 2021
@stealthybox stealthybox added kind/feature Categorizes issue or PR as related to a new feature. kind/enhancement Categorizes issue or PR as related to improving an existing feature. area/runtime Issues related to container runtimes and removed kind/feature Categorizes issue or PR as related to a new feature. labels Feb 2, 2021
@stealthybox stealthybox deleted the prefix-option branch March 22, 2021 14:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/runtime Issues related to container runtimes kind/enhancement Categorizes issue or PR as related to improving an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants