Skip to content
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

updata/update_metadata: Adds ability to add waves using TOML files #883

Merged
merged 1 commit into from
Apr 3, 2020

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Mar 30, 2020

Issue number:
Fixes #596

Description of changes:
This commit adds to updata the ability to use TOML-formatted files
to add waves to an existing update. The --wave-file flag for
the add-wave subcommand allows a user to specify the path to the
file where waves are defined.

Structs for deserializing the TOML have been added to update_metadata. A method on add_waves was also added Manifest. This allows us to update the manifest.json using other Rust code.

Testing done:

  • All unit tests continue to pass
  • An built image boots locally
  • Calling updata with a waves file with an invalid fleet_percentage fails as expected.
$ cat ./waves 
[[waves]]
start_after = 'in 12 hours'
fleet_percentage = 1

[[waves]]
start_after = 'in 1 day'
fleet_percentage = 10

[[waves]]
start_after = 'in 3 days'
fleet_percentage = 0

[[waves]]
start_after = 'in 6 days'
fleet_percentage = 100

$ cargo run --bin updata -- set-waves foobar --arch x86_64 --version 0.3.1 --variant aws-k8s-1.15 --wave-file ./waves
...
15:31:56 [ERROR] `fleet_percentage` must be a value between 1 - 100

$ cat ./waves 
[[waves]]
start_after = 'in 12 hours'
fleet_percentage = 1

[[waves]]
start_after = 'in 1 day'
fleet_percentage = 10

[[waves]]
start_after = 'in 3 days'
fleet_percentage = 101

[[waves]]
start_after = 'in 6 days'
fleet_percentage = 100

$ cargo run --bin updata -- set-waves foobar --arch x86_64 --version 0.3.1 --variant aws-k8s-1.15 --wave-file ./waves
...
15:33:17 [ERROR] `fleet_percentage` must be a value between 1 - 100

  • Calling updata with waves out of order fails as expected
$ cat ./waves 
[[waves]]
start_after = 'in 1 day'
fleet_percentage = 1

[[waves]]
start_after = 'in 12 hours'
fleet_percentage = 10

[[waves]]
start_after = 'in 3 days'
fleet_percentage = 25

[[waves]]
start_after = 'in 6 days'
fleet_percentage = 100

$ RUST_BACKTRACE=1 cargo run --bin updata -- set-waves foobar --arch x86_64 --version 0.3.1 --variant aws-k8s-1.15 --wave-file ./waves
...
15:35:59 [ERROR] Waves are not ordered: bound 20 occurs before bound 0
  • Calling updata with a valid waves file usin a fake manifest.json works a treat:
$ cat foobar
{
  "updates": [
    {
      "variant": "aws-k8s-1.15",
      "arch": "x86_64",
      "version": "0.3.0",
      "max_version": "0.3.1",
      "waves": {},
      "images": {
        "boot": "bottlerocket-aws-k8s-1.15-x86_64-0.3.0-faaec6e4-boot.ext4.lz4",
        "root": "bottlerocket-aws-k8s-1.15-x86_64-0.3.0-faaec6e4-root.ext4.lz4",
        "hash": "bottlerocket-aws-k8s-1.15-x86_64-0.3.0-faaec6e4-root.verity.lz4"
      }
    },
  ],
  "migrations": {}
}

$ cat ./waves
[[waves]]
start_after = 'in 12 hours'
fleet_percentage = 1

[[waves]]
start_after = 'in 1 day'
fleet_percentage = 10

[[waves]]
start_after = 'in 3 days'
fleet_percentage = 25

[[waves]]
start_after = 'in 6 days'
fleet_percentage = 100

$ cargo run --bin updata -- set-waves foobar --arch x86_64 --version 0.3.0 --variant aws-k8s-1.15 --wave-file ./waves
...

$ cat foobar
{
  "updates": [
    {
      "variant": "aws-k8s-1.15",
      "arch": "x86_64",
      "version": "0.3.0",
      "max_version": "0.3.1",
      "waves": {
        "0": "2020-04-03T11:08:59.483930025Z",
        "20": "2020-04-03T23:08:59.483949148Z",
        "204": "2020-04-05T23:08:59.483958597Z",
        "512": "2020-04-08T23:08:59.483967129Z"
      },
      "images": {
        "boot": "bottlerocket-aws-k8s-1.15-x86_64-0.3.0-faaec6e4-boot.ext4.lz4",
        "root": "bottlerocket-aws-k8s-1.15-x86_64-0.3.0-faaec6e4-root.ext4.lz4",
        "hash": "bottlerocket-aws-k8s-1.15-x86_64-0.3.0-faaec6e4-root.verity.lz4"
      }
    },
  ],
  "migrations": {}
}

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@zmrow zmrow self-assigned this Mar 30, 2020
@zmrow zmrow requested a review from bcressey March 30, 2020 19:31
@zmrow
Copy link
Contributor Author

zmrow commented Apr 2, 2020

Oops - fixed a tangling test data file in the wrong format and fixed the test.

tjkirch
tjkirch previously approved these changes Apr 2, 2020
Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

We still do the same checks on waves, right? That they're in order, seed doesn't go down, etc.

sources/parse-datetime/README.md Outdated Show resolved Hide resolved
sources/parse-datetime/src/lib.rs Outdated Show resolved Hide resolved
sources/updater/update_metadata/src/lib.rs Outdated Show resolved Hide resolved
sources/updater/update_metadata/src/lib.rs Outdated Show resolved Hide resolved
@tjkirch tjkirch self-requested a review April 2, 2020 23:41
@tjkirch tjkirch dismissed their stale review April 2, 2020 23:42

I'm confused about testing, going to ask about that

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

Your testing in the description has fleet_percentage = 0 which should be rejected, I think, not only because 0 is explicitly rejected but because the waves are out of order... can you please update the testing, and make sure we're doing proper wave sanity checks?

@zmrow
Copy link
Contributor Author

zmrow commented Apr 3, 2020

Gah - sorry about that. I was using a file with fleet_percentage = 0 when manually testing to ensure the program choked when it saw a zero. I'll fix the testing section of the description.

In regards to update order, I haven't changed the validation we're doing. update_metadata includes a function validate_updates which makes sure that the dates are sane and in ascending order. I only removed the ordered_waves() test because the purpose of the test is already covered in the set_waves() test and is therefore redundant.

Let me know if you have any other questions.

@zmrow
Copy link
Contributor Author

zmrow commented Apr 3, 2020

Updated to address @tjkirch 's comments.

Also updated the main description with additional testing.

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

No blockers, but I think it'd be good to confirm that seeds don't go down, just like we confirm times don't go down. That could prevent some issues where users think the percentages are incremental from the previous value, perhaps.

sources/updater/update_metadata/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sam-aws sam-aws left a comment

Choose a reason for hiding this comment

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

🏄‍♂️

This commit adds to updata the ability to use TOML-formatted files
to add waves to an existing update. The `--wave-file` flag for
the `add-wave` subcommand allows a user to specify the path to the
file where waves are defined.
@zmrow
Copy link
Contributor Author

zmrow commented Apr 3, 2020

Improve error messages for unordered waves and invalid fleet_percentage.

Add an additional comment to the validate_updates function that explains we will catch dates and fleet_percentages that are out of order.

@zmrow zmrow merged commit a160257 into bottlerocket-os:develop Apr 3, 2020
@zmrow zmrow deleted the default-waves branch April 3, 2020 21:36
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.

Create default update wave strategy
4 participants