Skip to content

✨ Backport "Allow disable automated cleaning"#239

Merged
metal3-io-bot merged 3 commits into
metal3-io:masterfrom
Nordix:disk-cleaning-type-v1a4/feruz
Jul 8, 2021
Merged

✨ Backport "Allow disable automated cleaning"#239
metal3-io-bot merged 3 commits into
metal3-io:masterfrom
Nordix:disk-cleaning-type-v1a4/feruz

Conversation

@fmuyassarov
Copy link
Copy Markdown
Member

@fmuyassarov fmuyassarov commented Jul 3, 2021 •
edited
Loading

What this PR does / why we need it:
Backport automated cleaning feature to the master.

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 3, 2021
@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch from 78a5026 to 6ebd0ec Compare July 3, 2021 11:46
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 3, 2021
@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch from 6ebd0ec to 2a21e68 Compare July 5, 2021 10:03
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 5, 2021
@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch from 2a21e68 to 0a052c1 Compare July 5, 2021 10:42
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 5, 2021
@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch 2 times, most recently from 82708ac to a0847f0 Compare July 5, 2021 14:12
@metal3-io-bot metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 5, 2021
@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch from a0847f0 to 08ea2c9 Compare July 6, 2021 21:04
@fmuyassarov
Copy link
Copy Markdown
Member Author

fmuyassarov commented Jul 6, 2021

/test-integration

@fmuyassarov fmuyassarov changed the title ✨ Backport automated cleaning field to v1a5 ✨ Backport "Allow disable automated cleaning" Jul 6, 2021
Xenwar
Xenwar reviewed Jul 7, 2021
View reviewed changes
Comment thread api/v1alpha4/metal3machine_types.go Outdated
Comment thread api/v1alpha5/metal3machine_types.go
// during provisioning and deprovisioning.
// +kubebuilder:default:=metadata
// +kubebuilder:validation:Enum:=metadata;disabled
AutomatedCleaningMode string `json:"automatedCleaningMode,omitempty"`
Copy link
Copy Markdown
Member

@Xenwar Xenwar Jul 7, 2021

Choose a reason for hiding this comment

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

same as above, Field name is not descriptive.
AutomatedDiskCleaningMode ?

Copy link
Copy Markdown
Member Author

@fmuyassarov fmuyassarov Jul 7, 2021

Choose a reason for hiding this comment

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

This is has been already implemented as AutomatedDiskCleaningMode, so changing it here means I need to replace it in BMO as well and in release-0.4 branch. I agree that it might not fully explain what it does, but for that reason I've added documentation.

Comment thread baremetal/metal3machine_manager.go Outdated
Comment thread baremetal/metal3machinetemplate_manager.go Outdated
Comment thread baremetal/metal3machinetemplate_manager.go Outdated
Comment thread api/v1alpha5/metal3machine_types.go
// during provisioning and deprovisioning.
// +kubebuilder:default:=metadata
// +kubebuilder:validation:Enum:=metadata;disabled
AutomatedCleaningMode string `json:"automatedCleaningMode,omitempty"`
Copy link
Copy Markdown
Member

@Xenwar Xenwar Jul 7, 2021

Choose a reason for hiding this comment

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

Field name is not descriptive enough, AutomatedCleaningMode
What is it cleaning ? disk ?
Which disk is it cleaning ? non-boot disk ?

Copy link
Copy Markdown
Member Author

@fmuyassarov fmuyassarov Jul 7, 2021

Choose a reason for hiding this comment

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

It is about secondary storage (i.e. hosted storage). I think having a good documentation will answer those questions.

Comment thread baremetal/metal3machinetemplate_manager.go Outdated
Comment thread baremetal/metal3machinetemplate_manager.go Outdated
if err := m.client.Update(ctx, m3m); err != nil {
return errors.Wrapf(err, "failed to update metal3Machine: %s", m3m.Name)
}
m.Log.Info("Synchronized automatedCleaningMode field value between Metal3MachineTemplate %v/%v and Metal3MachineMachine %v/%v", m.Metal3MachineTemplate.Namespace, m.Metal3MachineTemplate.Name, m3m.Namespace, m3m.Name)
Copy link
Copy Markdown
Member

@Xenwar Xenwar Jul 7, 2021

Choose a reason for hiding this comment

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

No error may not guarantee proper state change. What if you do assertion on the returned object to see if the field has actually been changed ?

Copy link
Copy Markdown
Member Author

@fmuyassarov fmuyassarov Jul 7, 2021

Choose a reason for hiding this comment

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

Right, I added a conditional, do you think that would be good enough?

Copy link
Copy Markdown
Member

@Xenwar Xenwar Jul 8, 2021

Choose a reason for hiding this comment

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

Indeed. Looks good.

Comment thread docs/api.md Outdated
Comment thread config/crd/bases/infrastructure.cluster.x-k8s.io_metal3machinetemplates.yaml Outdated
properties:
automatedCleaningMode:
default: metadata
description: When set to disabled, automated cleaning will
Copy link
Copy Markdown
Member

@Xenwar Xenwar Jul 7, 2021

Choose a reason for hiding this comment

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

See earlier comment. The two lines could be made to be on the same line

Copy link
Copy Markdown
Member Author

@fmuyassarov fmuyassarov Jul 7, 2021

Choose a reason for hiding this comment

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

Please, see my comment above

@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch from 08ea2c9 to cd4ed5e Compare July 7, 2021 18:28
@fmuyassarov fmuyassarov force-pushed the disk-cleaning-type-v1a4/feruz branch from cd4ed5e to b2929f3 Compare July 7, 2021 18:49
@fmuyassarov
Copy link
Copy Markdown
Member Author

fmuyassarov commented Jul 7, 2021

/cc @furkatgofurov7

@Xenwar
Copy link
Copy Markdown
Member

Xenwar commented Jul 8, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
@furkatgofurov7
Copy link
Copy Markdown
Member

furkatgofurov7 commented Jul 8, 2021

/test-integration

furkatgofurov7
furkatgofurov7 approved these changes Jul 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Jul 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2021
@metal3-io-bot metal3-io-bot merged commit 8959187 into metal3-io:master Jul 8, 2021
@fmuyassarov fmuyassarov deleted the disk-cleaning-type-v1a4/feruz branch August 14, 2021 19:40
@Rozzii Rozzii mentioned this pull request Dec 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Xenwar Xenwar Xenwar left review comments

@furkatgofurov7 furkatgofurov7 furkatgofurov7 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fmuyassarov @Xenwar @furkatgofurov7 @metal3-io-bot