-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added metadata cloud config support to CD ROM and openstack config drive providers #5
base: main
Are you sure you want to change the base?
Added metadata cloud config support to CD ROM and openstack config drive providers #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind amending the commit to include the Singed-off-by
footer in the commit message?
To avoid having PRs blocked in the future, always include Signed-off-by: Author Name authoremail@example.com in every commit message. You can also do this automatically by using the -s flag (i.e., git commit -s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this will be enough, but looks good to me adding support for meta-data
providers/provider_cdrom.go
Outdated
@@ -28,7 +28,7 @@ import ( | |||
// ListCDROMs lists all the cdroms in the system | |||
func ListCDROMs() []Provider { | |||
// UserdataFiles is where to find the user data | |||
var userdataFiles = []string{"user-data", "config"} | |||
var userdataFiles = []string{"user-data", "meta-data", "config"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to rancher/elemental-toolkit#2125 both files (user-data
and metadata
) are created for the Harvester use case. However the change here will only honor first match. I am not sure this is what we want. I am fine with the change proposed here to also check for meta-data
, however the logic of this datasource only reads the first match (see snipped below), as soon as file is read stops trying the next one. So if both files are available at the same time we might need so more elaborated change.
@robertwbl what you think? does it make sense to you? I have the feeling this change is not enough after reading the issue you created and checking how this userdataFiles
slice is being used.
linuxkit/providers/cdrom_shared.go
Lines 80 to 101 in fa64b2f
func NewProviderCDROM(device string, datafiles []string, providerType string) *ProviderCDROM { | |
mountPoint, err := os.MkdirTemp("", "cd") | |
p := ProviderCDROM{providerType, device, mountPoint, err, []byte{}} | |
if err == nil { | |
if p.err = p.mount(); p.err == nil { | |
defer p.unmount() | |
// read the userdata - we read the spec file and the fallback, but eventually | |
// will remove the fallback | |
for _, f := range datafiles { | |
userdata, err := os.ReadFile(path.Join(p.mountPoint, f)) | |
// did we find a file? | |
if err == nil && userdata != nil { | |
p.userdata = userdata | |
break | |
} | |
} | |
if p.userdata == nil { | |
log.Debugf("no userdata file found at any of %v", datafiles) | |
} | |
} | |
} | |
return &p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good spot - I hadn't read the usage clearly! You're right that this wouldn't work since Harvester provides both meta-data and user-data separately and simultaneously. I'll have a think about how this might work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just been exploring the original linuxkit repo this was forked from and it seems that has a way of handling the meta-data file: https://github.com/linuxkit/linuxkit/blob/be7dfdd42c6cc9365079f101b01536062729eda7/pkg/metadata/provider_cdrom.go#L121
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks as though in this fork, that behaviour got factored out in #1 . @davidcassany your commit message on 9106b9d makes it sounds like at the time it was just considered to be unused and there's no other reason it was removed? I'd be keen to factor it back in this way if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've factored this back in in a way that also considers the open stack config drive provider. According to https://docs.openstack.org/nova/rocky/user/config-drive.html it can also be expected to find a meta_data.json
file here. Let me know what you think, thanks!
…ovider_cdrom Signed-off-by: Robert Loveless <robert@rwbl.co.uk>
6de3e4e
to
6d1771c
Compare
…ding for openstack config drive provider. Signed-off-by: Robert Loveless <robert@rwbl.co.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this looks good to me.
However I still think this is not enough as this doesn't change anything to the providers interface, there is no method to extract the metadata from the interface.
Some options would be adding an extra method ExtractMetadata
to the interface and simply return empty data for providers not having any metadata
. Another option, if we are certain that this meta-data is only going to provide hostname and some limited set of variables, we could follow a similar approach as it is in done with most public cloud providers. Serveral public cloud providers write the hostname to a specific file that Elemental Toolkit consumes. See
linuxkit/providers/provider.go
Lines 25 to 26 in ac35c7b
// Hostname is the filename in configPath where the hostname is stored | |
Hostname = "hostname" |
I am trying to figure out the format and syntax of meta-data file now.
In any case I think your PR is a great base for the final solution.
Ok after finding the related cloud-init docs here I am starting to consider that we probably should only read the hostname form the metadata and use the same mechanism that is used for other cloud providers in this library. @robertwbl does it make sense to you? Do you feel like providing such an implementation? I can try to help if needed or even take it from this current PR if you just prefer us to finalize it. |
Continued from rancher/elemental-toolkit#2126