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.

Sign up for GitHub

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

Jump to bottom

MGMT-9549:Trigger for remote deploy HW specs #1518

Merged
merged 1 commit into from
Mar 13, 2022
Merged

MGMT-9549:Trigger for remote deploy HW specs #1518

merged 1 commit into from
Mar 13, 2022

Conversation

paul-maidment
Copy link
Contributor

It has been observed that while testing against production, there is an
issue with disk validation on workers. It has been found that setting
the disk size of a worker to 128GB resolves the issue. This trigger has
been added to ensure that this happens.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2022
@openshift-ci openshift-ci bot requested review from osherdp and tsorya March 9, 2022 10:34
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2022
osherdp
osherdp suggested changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@osherdp osherdp left a comment

Choose a reason for hiding this comment

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

great idea!

src/triggers/default_triggers.py Outdated
@@ -8,6 +8,18 @@

_default_triggers = frozendict(
{
"production": Trigger(
condition=("service_base_url", consts.RemoteEnvironment.PRODUCTION),
worker_disk=128849018880
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
worker_disk=128849018880
worker_disk=120 * 2 ** 30 # 120 GB

or any other option that will make the size in use more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

And please keep that value as const

@paul-maidment paul-maidment force-pushed the MGMT-9549 branch 2 times, most recently from 20c582a to a2cc416 Compare March 9, 2022 14:00
osherdp
osherdp suggested changes Mar 9, 2022
View reviewed changes
src/triggers/default_triggers.py Outdated
Comment on lines 11 to 13
"production_service_url": Trigger(
condition=("remote_service_url", consts.RemoteEnvironment.PRODUCTION),
worker_disk = consts.DISK_SIZE_120GB
),
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the nagging, but maybe same as the others:

Suggested change
"production_service_url": Trigger(
condition=("remote_service_url", consts.RemoteEnvironment.PRODUCTION),
worker_disk = consts.DISK_SIZE_120GB
),
"production": Trigger(
condition=("remote_service_url", consts.RemoteEnvironment.PRODUCTION),
worker_disk = consts.DISK_SIZE_120GB
),

src/tests/global_variables/default_variables.py Outdated
@@ -58,4 +60,4 @@ def get_api_client(self, offline_token=None, **kwargs) -> InventoryClient:
if not url:
url = utils.get_local_assisted_service_url(self.namespace, "assisted-service", self.deploy_target)

return ClientFactory.create_client(url, offline_token, **kwargs)
return ClientFactory.create_client(url, offline_token, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add blank lines at the end of files?

@paul-maidment paul-maidment force-pushed the MGMT-9549 branch 2 times, most recently from 567adbd to ed2d6d9 Compare March 9, 2022 16:18
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 9, 2022
@paul-maidment
Copy link
Contributor Author

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2022
@paul-maidment
Copy link
Contributor Author

/retest

@osherdp
Copy link
Contributor

osherdp commented Mar 10, 2022

@paul-maidment I guess it's no longer WIP, right?

@paul-maidment
MGMT-9549:Trigger for remote deploy HW specs
00bcbb4 
It has been observed that while testing against production, there is an
issue with disk validation on workers. It has been found that setting
the disk size of a worker to 128GB resolves the issue. This trigger has
been added to ensure that this happens.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2022
@openshift-ci
Copy link

openshift-ci bot commented Mar 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: osherdp, paul-maidment

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

The pull request process is described here

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 lgtm Indicates that a PR is ready to be merged. label Mar 10, 2022
@paul-maidment paul-maidment changed the title WIP:MGMT-9549:Trigger for remote deploy HW specs MGMT-9549:Trigger for remote deploy HW specs Mar 10, 2022
@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 Mar 10, 2022
@paul-maidment
Copy link
Contributor Author

/retest

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@osherdp
Copy link
Contributor

osherdp commented Mar 11, 2022

/hold
live-iso job fails consistently, so no reason to waste resources

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2022
@paul-maidment
Copy link
Contributor Author

/retest

@osherdp
Copy link
Contributor

osherdp commented Mar 13, 2022

/test e2e-metal-single-node-live-iso
should be fixed by now

@osherdp
Copy link
Contributor

osherdp commented Mar 13, 2022

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2022
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link

openshift-ci bot commented Mar 13, 2022

@paul-maidment: all tests passed!

Full PR test history. Your PR dashboard.

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.

@openshift-merge-robot openshift-merge-robot merged commit 79302be into openshift:master Mar 13, 2022
@osherdp osherdp mentioned this pull request Apr 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eliorerz eliorerz eliorerz left review comments

@osherdp osherdp osherdp approved these changes

@tsorya tsorya Awaiting requested review from tsorya

Assignees

@osherdp osherdp

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@paul-maidment @osherdp @openshift-bot @eliorerz @openshift-merge-robot