Add cloud-init degraded state and machine readable degraded errors to cloud-init status

[holmanb]

(Rebase merge)

Proposed Commit Message

See individual commit messages for details.

Additional Context

Design spec

TODO

• Docs to come in a followup PR, based on the commit message of bbf644d.

[TheRealFalcon]

Don't forget about cloud-init status --wait. If I have an error in my cloud-config, it currently returns status: degraded running, but does not wait.

[TheRealFalcon]

What do you think about presenting more information in the cloud-init status --long case? It's using the --long flag, so I'd expect "long" output.

detail:
Degraded functionality in stage [ init init-local modules-config modules-final]

doesn't really tell me anything useful except that something went wrong in each of the stages. At this point I'd have to either jump into the logs or know to run --format json to get all of the errors in json format. Can we present the errors here instead?

[holmanb]

What do you think about presenting more information in the cloud-init status --long case? It's using the --long flag, so I'd expect "long" output.

Sure, I can add that.

detail:
Degraded functionality in stage [ init init-local modules-config modules-final]

Just noticed this was buggy and was printing all stages if there was an error in any stage. I'll fix that.

Can we present the errors here instead?

I can add this to --long in addition to, but do not want to add this to --long instead of machine readable output.

[TheRealFalcon]

--long errors look good now!

I'm not sure I understand the structure of --json output:

{
  "_schema_version": "1",
  "boot_status_code": "enabled-by-generator",
  "datasource": "lxd",
  "detail": "DataSourceLXD",
  "errors": [],
  "last_update": "Wed, 11 Oct 2023 20:32:01 +0000",
  "recoverable_errors": {},
  "schemas": {
    "1": {
      "boot_status_code": "enabled-by-generator",
      "datasource": "lxd",
      "detail": "DataSourceLXD",
      "errors": [],
      "last_update": "Wed, 11 Oct 2023 20:32:01 +0000",
      "recoverable_errors": {},
      "status": "done",
      "v1": {
        "datasource": "DataSourceLXD",
        "init": {
          "errors": [],
          "exported_errors": {
            "WARNING": [
              "Failed at merging in cloud config part from part-001: empty cloud config"
            ]
          },
          "finished": 1697056316.532762,
          "start": 1697056316.0571733
        },
        "init-local": {
          "errors": [],
          "exported_errors": {},
          "finished": 1697056315.0196795,
          "start": 1697056314.8316493
        },
        "modules-config": {
          "errors": [],
          "exported_errors": {},
          "finished": 1697056320.82747,
          "start": 1697056320.6863594
        },
        "modules-final": {
          "errors": [],
          "exported_errors": {},
          "finished": 1697056321.1340263,
          "start": 1697056321.057343
        },
        "stage": null
      }
    }
  },
  "status": "done",
  "v1": {
    "datasource": "DataSourceLXD",
    "init": {
      "errors": [],
      "exported_errors": {
        "WARNING": [
          "Failed at merging in cloud config part from part-001: empty cloud config"
        ]
      },
      "finished": 1697056316.532762,
      "start": 1697056316.0571733
    },
    "init-local": {
      "errors": [],
      "exported_errors": {},
      "finished": 1697056315.0196795,
      "start": 1697056314.8316493
    },
    "modules-config": {
      "errors": [],
      "exported_errors": {},
      "finished": 1697056320.82747,
      "start": 1697056320.6863594
    },
    "modules-final": {
      "errors": [],
      "exported_errors": {},
      "finished": 1697056321.1340263,
      "start": 1697056321.057343
    },
    "stage": null
  }
}

Why is there a v1 that lives under schemas["1"].

Also, this is probably more relevant to one of the previous PRs, but in playing around with how different types of errors are handled, I noticed that not all warnings are being handled. The easiest way to demonstrate is to add a LOG.warning("This isn't showing up...") as the first line of the handle function of cc_bootcmd.py. For some reason this doesn't get reported by cloud-init status --long, though replacing it with a LOG.error does. I think we'll need this fixed in a follow-up PR along with an integration test showing that all errors/warnings from the logs make it into the status json.

[holmanb]

Why is there a v1 that lives under schemas["1"].

v1 groups together the stages and their error/start/end/recoverable_errors content

As for why it is under schemas["1"], I'm really not sure. Everything under schemas["1"] is a copy of the top level keys. I don't know why that approach was decided initially, but at this point I would rather be consistent rather than diverge without reason[1].

Also, this is probably more relevant to one of the previous PRs, but in playing around with how different types of errors are handled, I noticed that not all warnings are being handled. The easiest way to demonstrate is to add a LOG.warning("This isn't showing up...") as the first line of the handle function of cc_bootcmd.py. For some reason this doesn't get reported by cloud-init status --long, though replacing it with a LOG.error does. I think we'll need this fixed in a follow-up PR along with an integration test showing that all errors/warnings from the logs make it into the status json.

Yikes. I'll take a look, thanks for the heads up. That will have to be a follow-up PR.

[1] Aside: Personally I think we could blow away this whole versioning in json thing we've got going on in status.json, cloud-init status --format json, etc, (if not for backwards compatibility concerns) and have less nested keys and noise to deal with (when was the last time we removed a v1 of... well anything really?).

[TheRealFalcon]

v1 groups together the stages and their error/start/end/recoverable_errors content

But presumably schemas["1"] is there to indicate this is v1, so it's confusing to have an extra layer of versioning.

Per your aside, I was going to comment something similar but thought it not relevant enough to this PR. I have a few thoughts:
If we do need to version things, then we shouldn't have it duplicated at the top-level, and we shouldn't have multiple layers. E.g., rather than (truncated)

{
  "_schema_version": "1",
  "datasource": "lxd",
  "errors": [],
  "schemas": {
    "1": {
      "datasource": "lxd",
      "errors": []
    }
  }
}

Instead be

{
  "v1": {
    "datasource": "lxd",
    "errors": []
  }
}

If we don't want to version things, and then later need to introduce a breaking change, we can always version at that time, though it adds some ugliness. E.g.:

{
  "datasource": "lxd",
  "errors": [],
  "v2": {
    "datasource": "lxd",
    "errors": {...}
}

The current approach doesn't really buy us anything and just makes the output more cumbersome to work with. I'm fine either way, but I agree that we're unlikely to change the key types and can instead always add more keys. The problem is this has been in the wild for a few releases now, so it'd be a breaking change to change it now. It's probably worth doing, but if we do go ahead with it, we should also look at changing cloud-init query as that output has gotten unwieldly there too.

[cjp256]

Awesome! Access to the spec is restricted.

[holmanb]

If we do need to version things, then we shouldn't have it duplicated at the top-level, and we shouldn't have multiple layers. E.g., rather than (truncated)

Thanks for the discussion on this @TheRealFalcon.

Per out out of band discussion, I've gotten rid of the versioned "1": {} output in 61647e4. Ubuntu releases will need that change patched out for to avoid the breaking change.

I also dropped the v1 key from status.json and opted to include the contents of that key instead. There was a collision with the datasource key, but the code prunes that before merging, so we have a much cleaner output now:

# cloud-init status --format json
{
  "boot_status_code": "enabled-by-generator",
  "datasource": "nocloud",
  "detail": "DataSourceNoCloud [seed=/var/lib/cloud/seed/nocloud-net][dsmode=net]",
  "errors": [],
  "extended_status": "degraded done",
  "init": {
    "errors": [],
    "finished": 1698279442.4062886,
    "recoverable_errors": {
      "WARNING": [
        "Failed at merging in cloud config part from part-001: empty cloud config"
      ]
    },
    "start": 1698279441.3664
  },
  "init-local": {
    "errors": [],
    "finished": 1698279439.4033117,
    "recoverable_errors": {},
    "start": 1698279438.9879673
  },
  "last_update": "Thu, 26 Oct 2023 00:17:31 +0000",
  "modules-config": {
    "errors": [],
    "finished": 1698279450.2446203,
    "recoverable_errors": {
      "WARNING": [
        "No template found in /etc/cloud/templates for template named sources.list.ubuntu.deb822",
        "No template found in /etc/cloud/templates for template named sources.list",
        "No template found, not rendering /etc/apt/sources.list.d/ubuntu.sources"
      ]
    },
    "start": 1698279449.9259806
  },
  "modules-final": {
    "errors": [],
    "finished": 1698279451.0209844,
    "recoverable_errors": {},
    "start": 1698279450.8273187
  },
  "recoverable_errors": {
    "WARNING": [
      "Failed at merging in cloud config part from part-001: empty cloud config",
      "No template found in /etc/cloud/templates for template named sources.list.ubuntu.deb822",
      "No template found in /etc/cloud/templates for template named sources.list",
      "No template found, not rendering /etc/apt/sources.list.d/ubuntu.sources"
    ]
  },
  "stage": null,
  "status": "done"
}

[holmanb]

Awesome! Access to the spec is restricted.

@cjp256: I've moved the content of the spec to discourse:

https://discourse.ubuntu.com/t/spec-improve-error-and-warning-visibility/39765

[TheRealFalcon]

Looks really good now, but I have a few more comments.

It looks like the --long output isn't distinguishing between recoverable errors and hard errors E.g., when I provide an invalid network-config:

root@me:~# cloud-init status --long
status: error
extended_status: error
boot_status_code: enabled-by-generator
last_update: Fri, 27 Oct 2023 15:19:35 +0000
detail:
DataSourceLXD
recoverable_errors:
WARNING:
	- Failed to rename devices: Failed to apply network config names: Unknown network config version: None
	- failed stage init-local

The network config error appears as a recoverable error, but it appears as both in --format json.

root@me:~# cloud-init status --format json
{
  "boot_status_code": "enabled-by-generator",
  "datasource": "lxd",
  "detail": "DataSourceLXD",
  "errors": [
    "Unknown network config version: None"
  ],
  "extended_status": "error",
  "init": {
    "errors": [],
    "finished": 1698419940.0766742,
    "recoverable_errors": {
      "WARNING": [
        "Failed to rename devices: Failed to apply network config names: Unknown network config version: None"
      ]
    },
    "start": 1698419939.4522545
  },
  "init-local": {
    "errors": [
      "Unknown network config version: None"
    ],
    "finished": 1698419819.0041544,
    "recoverable_errors": {
      "WARNING": [
        "failed stage init-local"
      ]
    },
    "start": 1698419818.9596815
  },
  "last_update": "Fri, 27 Oct 2023 15:19:35 +0000",
  "modules-config": {
    "errors": [],
    "finished": 1698419975.6157393,
    "recoverable_errors": {},
    "start": 1698419975.4693048
  },
  "modules-final": {
    "errors": [],
    "finished": 1698419975.9615448,
    "recoverable_errors": {},
    "start": 1698419975.8828495
  },
  "recoverable_errors": {
    "WARNING": [
      "Failed to rename devices: Failed to apply network config names: Unknown network config version: None",
      "failed stage init-local"
    ]
  },
  "stage": null,
  "status": "error"
}

If there's not an easy way to distinguish them, we should at least print the hard errors in the --long output.

Also, I think we should add to the spec how we're distinguishing between hard error vs recoverable error. I couldn't tell until I checked the source that it comes down to service failure.

For integration tests, I would like to see an integration test that intentionally generates a few warnings and verifies that they show up in the status output.
For unit tests, I see that test_status.py has been updated to reflect the new output, but we don't have any tests that generate any kind of errors/warnings and ensure the output is correct. I think we should add a test that does this.

[TheRealFalcon]

Nit:

Suggested change

	# For backwards compability, don't report degraded status here,
	# For backwards compatibility, don't report degraded status here,

[TheRealFalcon]

I don't think this is right anymore. status command doesn't output v1 anymore, correct?

[holmanb]

Correct, thank you.

[holmanb]

@TheRealFalcon Thanks for the review.

The network config error appears as a recoverable error, but it appears as both in --format json.

Hrm, you're right. I guess I can add both to --long

Also, I think we should add to the spec how we're distinguishing between hard error vs recoverable error. I couldn't tell until I checked the source that it comes down to service failure.

Happy to add that to the spec. I'm planning on basing the followup documentation PR on the commit message that implements this feature. Do you think the content there is sufficient regarding the difference between cloud-init failure and recoverable error, or would you prefer to see more/different content?

For integration tests, I would like to see an integration test that intentionally generates a few warnings and verifies that they show up in the status output.
For unit tests, I see that test_status.py has been updated to reflect the new output, but we don't have any tests that generate any kind of errors/warnings and ensure the output is correct. I think we should add a test that does this.

+1 Will do agreed. I didn't want to generate new tests until we were happy with how this looked to avoid unnecessary rework.

[holmanb]

Also, I think we should add to the spec how we're distinguishing between hard error vs recoverable error. I couldn't tell until I checked the source that it comes down to service failure.

See the new current state and future state sections under the Implementation section.

[TheRealFalcon]

See the new current state and future state sections under the Implementation section.

Those are great, thanks!

Do you think the content there is sufficient regarding the difference between cloud-init failure and recoverable error, or would you prefer to see more/different content?

I don't see what distinguishes a recoverable error from a non-recoverable one. I think if you make the same distinction that you made in the spec, that will work.

[holmanb]

I don't see what distinguishes a recoverable error from a non-recoverable one. I think if you make the same distinction that you made in the spec, that will work.

Thanks for the feedback, I'll add that to the commit message when I'm ready to merge.

Also, I think I've addressed the remaining concerns in ab35812. Better unittest and integration test coverage, fixed the spelling nit, and included both errors and recoverable errors in the output of --long.

Ready for re-review.