Fix snapd growpart and final message warnings #5215

blackboxsw · 2024-04-25T19:14:33Z

rebase merge use individual commits

Proposed Commit Message(s)

fix(final_message): do not warn on datasourcenone when single ds

Avoid warning level log "Used fallback datasource" when base
configuration defines datasource_list: [ None ].

Using DataSourceNone in this case is desired behavior.

Also drop unused is_disconnected property from DataSource
which was only used by DataSourceNone in cc_final_message.

Instead compare based on dsname property.

Fixes: Remove DataSourceNone warning in cc_final_message #5192
fix(growpart): correct growpart log message to include value of mode

Additional Context

Affects snapd UC24 which sets `datasource_list: [ None ]' in some configs

Test Steps

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

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

blackboxsw · 2024-04-25T19:18:39Z

cc: @Meulengracht for snap-related concerns.

Meulengracht · 2024-04-25T19:27:55Z

Thank you to everyone involved here for getting this ball rolling and fixed so quickly!

cloudinit/config/cc_growpart.py

holmanb

Hey @blackboxsw, thanks for this PR. I just added some wording nits to hopefully make our docs a bit clearer.

holmanb · 2024-04-30T18:16:32Z

cloudinit/config/cc_final_message.py

+            log_func = LOG.debug
+        else:
+            log_func = LOG.warning
+        log_func("Used fallback datasource")


Plenty of other logs in the log file by this point would have indicated that the user is using DatasourceNone, so I don't see what this adds as a debug message. Maybe just log a warning if this is not the expected behavior?

Good pt. droppng debug version of msg.

@blackboxsw I'm guessing this was forgotten?

cloudinit/config/schemas/schema-cloud-config-v1.json

doc/rtd/explanation/configuration.rst

holmanb · 2024-04-30T18:47:54Z

doc/rtd/explanation/configuration.rst

@@ -23,6 +23,11 @@ From lowest priority to highest, configuration sources are:

 These four sources make up the base configuration.

+.. note::
+   The base configuration files must be valid `YAML 1.1`_ syntax and they can
+   be declared as :ref:`jinja templates <instancedata-Using>`.


The base configuration files must be valid YAML 1.1_ syntax and they can be declared as :ref:jinja templates <instancedata-Using>.

A jinja template that contains a base configuration is not necessarily valid yaml, so how can this be true? This seems self-contradictory.

We work around this with ## template: jinja template headers, but this means that a base configuration file must be either YAML 1.1 or a jinja template using instance-data which, when rendered, produces a YAML 1.1 file.

Additionally, I don't think that the phrase "valid YAML 1.1 syntax" really accomplishes what I think you're trying to say. YAML 1.2 can be valid 1.1, but the point we're trying to make is that cloud-init uses the YAML 1.1 interpretation of the configuration.

Maybe something like this will help clarify this statement a bit?

The base configuration format uses `YAML version 1.1`_, but may be defined as jinja templates which cloud-init will render at runtime with :ref:`instance-data<instancedata-Using>` variables.

What do you think about making this content part of the first paragraph on this page?

doc/rtd/explanation/configuration.rst

holmanb · 2024-04-30T19:20:25Z

doc/rtd/explanation/format.rst

@@ -24,7 +24,7 @@ These things include:
 - *and many more...*

 .. note::
-   This file must be valid YAML syntax.
+   This file must be valid `YAML 1.1`_ syntax.


I know you didn't write this line, but while we're touching this line can we please update it to something that makes a little more sense?

The file isn't a syntax. YAML isn't a syntax either. YAML is a standardized language for defining data objects, and it has a syntax for describing these data objects as a stream of characters. Cloud-config is a format which uses the YAML syntax described in specification version 1.1 and a set of keys defined in the jsonschema.

This file ...

Additionally, this configuration is often times not even a file. Users may copy-paste a cloud-config into a web interface, for example, and ultimately on the instance they usually do not write a configuration file, so the mention of it being a file may be misleading to some.

Perhaps this paragraph:

Cloud-config is the simplest way to accomplish some things via user data.
Using cloud-config syntax, the user can specify certain things in a
human-friendly format.

Could be updated to say something like:

Cloud-config is the preferred user data format. The cloud config format is a
declarative syntax which uses YAML version 1.1 with keys that describe instance
state. Cloud-config can be used to define how an instance should be configured in
a human-friendly format.

And then remove the This file must be ... note?

I like your updated suggestions. I've pulled the majority into the first paragraph of Cloud-config section as well as rewriting a couple of the apt-specific examples to more package format packaging format agnostic terms. CC @s-makin for doc review as well.

doc/rtd/explanation/format.rst

doc/rtd/explanation/instancedata.rst

s-makin

I don't have much to add beyond a couple of nits

s-makin · 2024-05-03T08:44:33Z

doc/rtd/explanation/configuration.rst

@@ -79,3 +83,4 @@ images.

 .. _Config: https://github.com/canonical/cloud-init/blob/b861ea8a5e1fd0eb33096f60f54eeff42d80d3bd/cloudinit/settings.py#L22
 .. _cloud.cfg template: https://github.com/canonical/cloud-init/blob/main/config/cloud.cfg.tmpl
+.. _YAML 1.1: https://yaml.org/spec/1.1/current.html


Suggested change

.. _YAML 1.1: https://yaml.org/spec/1.1/current.html

.. _YAML version 1.1: https://yaml.org/spec/1.1/current.html

The mismatch is (I think) what's throwing the spellcheck error. Even though this isn't an error in the spelling.

doc/rtd/explanation/format.rst

holmanb

I've pulled the majority into the first paragraph of Cloud-config section as well as rewriting a couple of the apt-specific examples to more package format packaging format agnostic terms.

Great idea. This is looking much better! I left a couple more questions and suggestions inline.

doc/rtd/explanation/configuration.rst

doc/rtd/explanation/format.rst

github-actions · 2024-05-23T00:21:10Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

holmanb

I have no more updates to make, I'm just setting this to "request changes" to make it more obvious that this is waiting on @blackboxsw right now.

holmanb · 2024-05-31T23:59:07Z

I have no more updates to make, I'm just setting this to "request changes" to make it more obvious that this is waiting on @blackboxsw right now.

@blackboxsw since this will make DataSourceNone users no longer have a warning message we might want to squeeze this one into 24.2. I'm adding it to the milestone so that it doesn't slip by without a discussion (we can remove it if we decide it's not a priority).

blackboxsw

Thanks for the more specific text suggestions @holmanb . I've added them all.

blackboxsw · 2024-06-04T14:55:10Z

doc/rtd/explanation/format.rst

- additional ``apt`` sources should be added
- certain SSH keys should be imported
+- performing package upgrades on first boot
+- configuration of different package mirrors or sources


@holmanb here's the only unreviewed diff

holmanb

Thanks @blackboxsw! Let's land it.

Update growpart schema messaging clarify having to quote the value of growpart: {mode: 'off'}

Avoid warning level log "Used fallback datasource" when base configuration defines `datasource_list: [ None ]`. Using DataSourceNone in this case is desired behavior. Also drop unused is_disconnected property from DataSource which was only used by DataSourceNone in cc_final_message. Instead compare based on dsname property. Fixes: canonicalGH-5192

blackboxsw force-pushed the fix-snapd-growpart-and-final-message-warnings branch from a71721c to 5dc4b0b Compare April 25, 2024 19:17

holmanb reviewed Apr 25, 2024

View reviewed changes

cloudinit/config/cc_growpart.py Show resolved Hide resolved

blackboxsw force-pushed the fix-snapd-growpart-and-final-message-warnings branch 3 times, most recently from 52e64e9 to 6d5f33b Compare April 25, 2024 21:25

blackboxsw added the documentation This Pull Request changes documentation label Apr 26, 2024

holmanb self-assigned this Apr 30, 2024

holmanb reviewed Apr 30, 2024

View reviewed changes

blackboxsw requested a review from s-makin May 2, 2024 22:47

github-actions bot removed the documentation This Pull Request changes documentation label May 2, 2024

s-makin reviewed May 3, 2024

View reviewed changes

blackboxsw force-pushed the fix-snapd-growpart-and-final-message-warnings branch from 7994c54 to 9fcbb9d Compare May 7, 2024 14:03

blackboxsw requested a review from holmanb May 7, 2024 14:04

holmanb reviewed May 8, 2024

View reviewed changes

doc/rtd/explanation/configuration.rst Outdated Show resolved Hide resolved

doc/rtd/explanation/format.rst Outdated Show resolved Hide resolved

doc/rtd/explanation/format.rst Outdated Show resolved Hide resolved

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label May 23, 2024

github-actions bot closed this May 30, 2024

holmanb reopened this May 30, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label May 30, 2024

holmanb requested changes May 31, 2024

View reviewed changes

holmanb added this to the cloud-init-24.2 milestone May 31, 2024

blackboxsw commented Jun 3, 2024

View reviewed changes

blackboxsw requested a review from holmanb June 3, 2024 16:03

blackboxsw commented Jun 4, 2024

View reviewed changes

holmanb reviewed Jun 5, 2024

View reviewed changes

holmanb approved these changes Jun 5, 2024

View reviewed changes

fix(growpart): correct growpart log message to include value of mode

5821284

Update growpart schema messaging clarify having to quote the value of growpart: {mode: 'off'}

blackboxsw added 2 commits June 5, 2024 05:38

doc: update docs adding YAML 1.1 spec and jinja template references

e2c6fc9

blackboxsw force-pushed the fix-snapd-growpart-and-final-message-warnings branch from 4767da3 to e2c6fc9 Compare June 5, 2024 11:38

blackboxsw merged commit fbec4d0 into canonical:main Jun 5, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix snapd growpart and final message warnings #5215

Fix snapd growpart and final message warnings #5215

blackboxsw commented Apr 25, 2024

blackboxsw commented Apr 25, 2024

Meulengracht commented Apr 25, 2024

holmanb left a comment

holmanb Apr 30, 2024

blackboxsw May 1, 2024

holmanb May 8, 2024

holmanb Apr 30, 2024

holmanb Apr 30, 2024

blackboxsw May 2, 2024

s-makin left a comment

s-makin May 3, 2024

s-makin May 3, 2024

holmanb left a comment

github-actions bot commented May 23, 2024

holmanb left a comment

holmanb commented May 31, 2024

blackboxsw left a comment

blackboxsw Jun 4, 2024

holmanb left a comment

	.. _YAML 1.1: https://yaml.org/spec/1.1/current.html
	.. _YAML version 1.1: https://yaml.org/spec/1.1/current.html

Fix snapd growpart and final message warnings #5215

Fix snapd growpart and final message warnings #5215

Conversation

blackboxsw commented Apr 25, 2024

rebase merge use individual commits

Proposed Commit Message(s)

Additional Context

Test Steps

Checklist

Merge type

blackboxsw commented Apr 25, 2024

Meulengracht commented Apr 25, 2024

holmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-makin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2024

holmanb left a comment

Choose a reason for hiding this comment

holmanb commented May 31, 2024

blackboxsw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment