Skip to content

USHIFT-1083 Back up data if previous boot was healthy#1826

Closed
pmtk wants to merge 9 commits intoopenshift:mainfrom
pmtk:ostree-health-history
Closed

USHIFT-1083 Back up data if previous boot was healthy#1826
pmtk wants to merge 9 commits intoopenshift:mainfrom
pmtk:ostree-health-history

Conversation

@pmtk
Copy link
Member

@pmtk pmtk commented May 22, 2023

No description provided.

@pmtk
Copy link
Member Author

pmtk commented May 22, 2023

/assign @dhellmann @pacevedom

@openshift-ci openshift-ci bot requested review from dhellmann and pliurh May 22, 2023 15:23
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmtk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2023
@pmtk pmtk changed the title USHIFT-1083 microshift admin ostree {get,set}-health + greenboot WIP USHIFT-1083 microshift admin ostree {get,set}-health + greenboot May 23, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2023
@pmtk pmtk force-pushed the ostree-health-history branch 6 times, most recently from 335a15f to 68efd88 Compare May 25, 2023 08:04
@pmtk pmtk mentioned this pull request May 26, 2023
Merged
@pmtk pmtk force-pushed the ostree-health-history branch 3 times, most recently from 5f0a5df to 7e1520c Compare May 26, 2023 16:02
@pmtk pmtk changed the title WIP USHIFT-1083 microshift admin ostree {get,set}-health + greenboot WIP USHIFT-1083 Boot history & pre-run May 26, 2023
@pmtk pmtk force-pushed the ostree-health-history branch 6 times, most recently from 9bf64a9 to 146fc77 Compare May 31, 2023 09:00
dhellmann
dhellmann reviewed May 31, 2023
View reviewed changes
packaging/greenboot/green.sh Outdated
Copy link
Contributor

@dhellmann dhellmann May 26, 2023

Choose a reason for hiding this comment

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

Are there other sub-commands to admin ostree besides set-health? If not, maybe we only need admin set-healthy and admin set-unhealthy.

Copy link
Member Author

@pmtk pmtk Jun 1, 2023

Choose a reason for hiding this comment

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

Initially there was supposed to be prerun subcommand, but I incorporated into run so I'll drop one level

Copy link
Member Author

@pmtk pmtk Jun 1, 2023

Choose a reason for hiding this comment

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

I made more changes: removed admin command, data and health are top level commands, health is hidden (not shown in help)

pkg/admin/history/file_storage.go Outdated
Copy link
Contributor

@dhellmann dhellmann May 31, 2023

Choose a reason for hiding this comment

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

It would be safer to write a new file then move it over the top of the existing file.

pkg/admin/prerun/advisor_backup.go Outdated
Copy link
Contributor

@dhellmann dhellmann May 31, 2023

Choose a reason for hiding this comment

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

Could this function just return false in this case, to make it safer to use?

Copy link
Member Author

@pmtk pmtk Jun 1, 2023

Choose a reason for hiding this comment

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

Maybe, not sure
Ideally these advisors would be more like "decision tree" structs to have everything "strongly typed", but it might not contribute to readability.
But for now I'd rather have this panic (at least until release) so it doesn't go unnoticed (i.e. this flow should be considered a fault in implementation), WDYT?

@pmtk pmtk force-pushed the ostree-health-history branch from decb76c to b889134 Compare June 1, 2023 06:43
@pmtk pmtk mentioned this pull request Jun 1, 2023
Merged
@pmtk
Copy link
Member Author

pmtk commented Jun 1, 2023 •
edited
Loading

@dhellmann I moved RF test (with comments) to #1866

@pmtk pmtk force-pushed the ostree-health-history branch 4 times, most recently from 6775dcc to db00a3b Compare June 1, 2023 11:15
@pmtk pmtk force-pushed the ostree-health-history branch from db00a3b to 294da29 Compare June 1, 2023 11:19
@pmtk pmtk changed the title WIP USHIFT-1083 Boot history & pre-run USHIFT-1083 Backup if previous boot was healthy Jun 1, 2023
@pmtk pmtk changed the title USHIFT-1083 Backup if previous boot was healthy USHIFT-1083 Backup data if previous boot was healthy Jun 1, 2023
@pmtk pmtk changed the title USHIFT-1083 Backup data if previous boot was healthy USHIFT-1083 Back up data if previous boot was healthy Jun 1, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 1, 2023

@pmtk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openshift-conformance-reduced-arm 28ed4df link false /test e2e-openshift-conformance-reduced-arm
ci/prow/microshift-e2e-arm 28ed4df link false /test microshift-e2e-arm
ci/prow/microshift-e2e 28ed4df link false /test microshift-e2e
ci/prow/e2e-openshift-conformance-reduced 28ed4df link true /test e2e-openshift-conformance-reduced

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dhellmann
dhellmann reviewed Jun 1, 2023
View reviewed changes
Copy link
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

This PR is growing pretty big, and it's doing a lot of things that don't seem to be in scope for the description of the original ticket.

Could we store the health state as a simple string in a file by itself? That would let the greenboot integration just be shell scripts that write to the file, and then MicroShift could read that file to check the state on startup and decide whether to do the backup. Then we could bring some of the rest of the accounting stuff for history in via separate PRs linked to some of the other tickets, where the code changes would be smaller and easier to review.

packaging/greenboot/set-health.sh

SCRIPT_DIR="$(readlink -f "$(dirname "${BASH_SOURCE[0]}")")"

if [[ "${SCRIPT_DIR}" == *"green.d"* ]]; then
Copy link
Contributor

@dhellmann dhellmann Jun 1, 2023

Choose a reason for hiding this comment

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

This feels tricky. Could we just have 2 scripts so we're explicitly passing the health status?

@pmtk
Copy link
Member Author

pmtk commented Jun 2, 2023

I'll split this PR into smaller pieces starting with #1874
/close

@openshift-ci openshift-ci bot closed this Jun 2, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 2, 2023

@pmtk: Closed this PR.

Details

In response to this:

I'll split this PR into smaller pieces starting with #1874
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pmtk pmtk deleted the ostree-health-history branch November 6, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhellmann dhellmann dhellmann left review comments

@pliurh pliurh Awaiting requested review from pliurh

Assignees

@dhellmann dhellmann

@pacevedom pacevedom

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pmtk @dhellmann @pacevedom