Skip to content

mon, qa: suites override ec profiles with --yes_i_really_mean_it; monitors accept that#56531

Merged
yuriw merged 2 commits intoceph:mainfrom
rzarzynski:wip-bug-65183
Jun 25, 2024
Merged

mon, qa: suites override ec profiles with --yes_i_really_mean_it; monitors accept that#56531
yuriw merged 2 commits intoceph:mainfrom
rzarzynski:wip-bug-65183

Conversation

@rzarzynski
Copy link
Contributor

@rzarzynski rzarzynski commented Mar 27, 2024 •
edited by ljflores
Loading

This fixes a fallout from 629ba7b. The problem has been nailed down by Laura Flores.

Fixes: https://tracker.ceph.com/issues/65183

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@github-actions github-actions bot added the tests label Mar 27, 2024
@ljflores
Copy link
Member

ljflores commented Apr 3, 2024

jenkins test api

@ljflores
Copy link
Member

ljflores commented Apr 3, 2024

@rzarzynski I saw this in a different test run, but this also needs to be fixed for qa/standalone/mon/osd-erasure-code-profile.sh:

/a/yuriw-2024-04-01_20:57:46-rados-wip-yuri3-testing-2024-04-01-0837-squid-distro-default-smithi/7634757

2024-04-02T04:26:16.310 INFO:tasks.workunit.client.0.smithi107.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/mon/osd-erasure-code-profile.sh:62: TEST_set:  grep 'will not override' td/osd-erasure-code-profile/out
2024-04-02T04:26:16.311 INFO:tasks.workunit.client.0.smithi107.stdout:Error EPERM: will not override erasure code profile myprofile because the existing profile {crush-device-class=,crush-failure-domain=host,crush-num-failure-domains=0,crush-osds-per-failure-domain=0,crush-root=default,k=7,key=value,m=3,plugin=isa,technique=reed_sol_van} is different from the proposed profile {crush-device-class=,crush-failure-domain=host,crush-num-failure-domains=0,crush-osds-per-failure-domain=0,crush-root=default,jerasure-per-chunk-alignment=false,k=2,m=2,plugin=jerasure,technique=reed_sol_van,w=8}
2024-04-02T04:26:16.311 INFO:tasks.workunit.client.0.smithi107.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/mon/osd-erasure-code-profile.sh:63: TEST_set:  ceph osd erasure-code-profile set myprofile key=other --force
2024-04-02T04:26:16.459 INFO:tasks.workunit.client.0.smithi107.stderr:Error EPERM: overriding erasure code profile can be DANGEROUS; add --yes-i-really-mean-it to do it anyway
2024-04-02T04:26:16.462 INFO:tasks.workunit.client.0.smithi107.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/mon/osd-erasure-code-profile.sh:63: TEST_set:  return 1

It can perhaps be done in a separate PR since this one is already in testing. WDYT?

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Apr 8, 2024

Fixing.

@rzarzynski rzarzynski requested a review from a team as a code owner April 8, 2024 15:39
@rzarzynski
Copy link
Contributor Author

rzarzynski commented Apr 8, 2024

@ljflores: now this should be also covered.

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Apr 8, 2024

Looks unrelated:

The following tests FAILED:
	 22 - run-tox-cephadm (Failed)

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Apr 8, 2024

jenkins test make check

ljflores
ljflores reviewed Apr 8, 2024
View reviewed changes
qa/workunits/cephtool/test.sh Outdated
ceph osd erasure-code-profile set fooprofile a=b c=d
expect_false ceph osd erasure-code-profile set fooprofile a=b c=d e=f
ceph osd erasure-code-profile set fooprofile a=b c=d e=f --force
ceph osd erasure-code-profile set fooprofile a=b c=d e=f --force --yes_i_really_mean_it
Copy link
Member

@ljflores ljflores Apr 8, 2024

Choose a reason for hiding this comment

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

Should be:

Suggested change
ceph osd erasure-code-profile set fooprofile a=b c=d e=f --force --yes_i_really_mean_it
ceph osd erasure-code-profile set fooprofile a=b c=d e=f --force --yes-i-really-mean-it

I pushed a small fix for this.

ljflores
ljflores reviewed Apr 16, 2024
View reviewed changes
qa/workunits/cephtool/test.sh
Comment on lines 2491 to 2492
ceph osd erasure-code-profile set fooprofile a=b c=d
expect_false ceph osd erasure-code-profile set fooprofile a=b c=d e=f
Copy link
Member

@ljflores ljflores Apr 16, 2024

Choose a reason for hiding this comment

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

From the latest results, it looks like we need to also add this flag to other lines of the script- basically wherever we're overriding an existing profile:

2024-04-12T01:54:30.858 INFO:tasks.workunit.client.0.smithi023.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2490: test_mon_osd_erasure_code:  ceph osd erasure-code-profile set fooprofile a=b c=d
2024-04-12T01:54:31.422 INFO:tasks.workunit.client.0.smithi023.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2491: test_mon_osd_erasure_code:  ceph osd erasure-code-profile set fooprofile a=b c=d
2024-04-12T01:54:31.835 INFO:tasks.workunit.client.0.smithi023.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2492: test_mon_osd_erasure_code:  expect_false ceph osd erasure-code-profile set fooprofile a=b c=d e=f

Copy link
Contributor Author

@rzarzynski rzarzynski Apr 17, 2024

Choose a reason for hiding this comment

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

This failure is pretty weird. The change should affect only the ceph osd erasure-code-profile set --force commands; those without --force should be failing (if the profiles differ) even in the past.

Do you have a teuthology.log maybe?

Also, I found two other cases in test_rados_tool.sh. I'm pushing updated commit because that.

@rzarzynski
qa: tests override ec profiles with --yes-i-really-mean-it
6f81323 
This fixes a fallout from 629ba7b.
The problem has been nailed down by Laura Flores.

Fixes: https://tracker.ceph.com/issues/65183
Signed-off-by: Radosław Zarzyński <rzarzyns@redhat.com>
@yuriw
Copy link
Contributor

yuriw commented May 3, 2024

This PR is under test in https://tracker.ceph.com/issues/65797.

@rzarzynski
Copy link
Contributor Author

rzarzynski commented May 6, 2024

jenkins test make check

1 similar comment
@rzarzynski
Copy link
Contributor Author

rzarzynski commented May 20, 2024

jenkins test make check

Matan-B
Matan-B previously requested changes May 23, 2024
View reviewed changes
Copy link
Contributor

@Matan-B Matan-B left a comment •
edited
Loading

Choose a reason for hiding this comment

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

https://pulpito.ceph.com/yuriw-2024-05-15_21:09:29-rados-wip-yuri5-testing-2024-05-15-0804-distro-default-smithi/7707707/

2024-05-15T22:27:06.733 INFO:tasks.workunit.client.0.smithi110.stderr:Error EPERM: overriding erasure code profile can be DANGEROUS; add --yes-i-really-mean-it to do it anyway

https://qa-proxy.ceph.com/teuthology/yuriw-2024-05-15_21:09:29-rados-wip-yuri5-testing-2024-05-15-0804-distro-default-smithi/7707787/teuthology.log

https://tracker.ceph.com/issues/65183#note-13

@Matan-B
Copy link
Contributor

Matan-B commented May 23, 2024

This PR only changes 2 test files, I think that we should test those separably in order to merge. (no need for full batch)

@github-actions github-actions bot added the mon label Jun 14, 2024
@rzarzynski rzarzynski changed the title qa: cephtool/test.sh overrides ec profile with --yes_i_really_mean_it mon, qa: suites override ec profiles with --yes_i_really_mean_it; monitors accept that Jun 14, 2024
@rzarzynski
Copy link
Contributor Author

rzarzynski commented Jun 14, 2024

@ljflores, @Matan-B: I think I know why the --yes-i-really-mean-it got ignored. Pushed a fix.

@ljflores ljflores removed the TESTED label Jun 17, 2024
@ljflores
Copy link
Member

ljflores commented Jun 17, 2024

Thanks @rzarzynski, we'll get this into a QA batch.

@Matan-B Matan-B dismissed their stale review June 18, 2024 06:54

not relevant anymore

@yuriw yuriw merged commit f939c35 into ceph:main Jun 25, 2024
@amathuria amathuria mentioned this pull request Jun 25, 2024
Merged
@k0ste k0ste mentioned this pull request Aug 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljflores ljflores ljflores approved these changes

@Matan-B Matan-B Matan-B left review comments

Assignees

No one assigned

Labels

core mon needs-qa tests wip-yuri11-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rzarzynski @ljflores @yuriw @Matan-B