Skip to content

Fix issue regarding cache and region codes#1938

Merged
holmanb merged 4 commits into
canonical:mainfrom
eb3095:vultr-region-fix
Jan 9, 2023
Merged

Fix issue regarding cache and region codes#1938
holmanb merged 4 commits into
canonical:mainfrom
eb3095:vultr-region-fix

Conversation

@eb3095
Copy link
Copy Markdown
Contributor

@eb3095 eb3095 commented Jan 6, 2023

Proposed Commit Message

Fix a bug related to the region being reassigned and breaking init-local when cache is used. 

Additional Context

image

Test Steps

  • Deploy vultr instance
  • Reboot

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@holmanb
Copy link
Copy Markdown
Member

holmanb commented Jan 6, 2023

I see this bug was introduced in 69f9a78. This datastructure is pickled, so we probably shouldn't be modifying the type of the data structure. What was wrong with the region code handling before 69f9a78? I don't see any explanation given in that PR, and with a quick scan I'm not seeing where that gets used.

@holmanb holmanb self-assigned this Jan 6, 2023
@eb3095
Copy link
Copy Markdown
Contributor Author

eb3095 commented Jan 7, 2023

The data structure we used for metadata prior to that commit changed. Before we assigned a dict with the VM's region's airport code only, but this was also wrong and broke several distros that use that to setup the repos such as Ubuntu, something we were requested to resolve. However the metadata is now provided as such,

Previously,

"region": {
  "regioncode": "EWR"
},

Now,

 "region": {
  "countrycode": "US",
  "regioncode": "EWR"
 },

region is expected to be a string so it cant just be dropped in like that. Note this fix has been tested and is working as expected and we are likely moving this patch into production for the time being.

As for it's usage, its used here,
https://github.com/canonical/cloud-init/blob/main/config/cloud.cfg.tmpl#L238

I personally don't really like this approach either. Changing our metadata however is unlikely to happen soon. Best alternative I can provide is I refactor this in the vultr.py helper and format the metadata into the expected cloud-init format before it hits this point. Would that be a better approach in your opinion?

@eb3095
Copy link
Copy Markdown
Contributor Author

eb3095 commented Jan 7, 2023

Eh, I went ahead and just did that. This is cleaner and feels better overall. I can revert however if its not ideal.

Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

This is cleaner and feels better overall.

Agreed. Putting the transformations in a function seems cleaner, and shifting the callsite to within get_metadata() avoids applying the transformation multiple times, which caused this bug.

@holmanb holmanb merged commit cb128dc into canonical:main Jan 9, 2023
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