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

Intelligent Host Selection from Platform Group #4329

Merged
merged 27 commits into from
Oct 4, 2021

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jul 29, 2021

These changes close #3827

Put up as draft - Functionally tested and probably working. Unit tests to be written.

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included.
    • Functional
    • Unit (For the get_platform_from_group function)
  • Appropriate change log entry included.
  • (master branch) I have opened a documentation PR at added a guide to writing a platform configuration. cylc-doc#289, covering the admin ramifications of this change.
  • (master branch) I have opened a documentation PR at Selection of hosts and platforms cylc-doc#299 (very much still wip)

added selection method to config for platform groups

functional tests for platform from group
@wxtim wxtim changed the title Ihs.from platform group Intelligent Host Selection from Platform Group Jul 29, 2021
@wxtim wxtim marked this pull request as draft July 29, 2021 08:31
@wxtim wxtim added this to the cylc-8.0b3 milestone Jul 29, 2021
@wxtim wxtim self-assigned this Jul 29, 2021
@wxtim wxtim marked this pull request as ready for review July 31, 2021 08:42
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(Had a quick look, looks good 👍 )

@hjoliver
Copy link
Member

(Resolved a small conflict on this branch via GH UI)

cylc/flow/exceptions.py Outdated Show resolved Hide resolved
cylc/flow/task_job_mgr.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

Could get coverage up a bit?

wxtim and others added 2 commits August 31, 2021 08:05
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
@wxtim
Copy link
Member Author

wxtim commented Aug 31, 2021

Could get coverage up a bit?

Always happy to try to do this.

But there may have been reasons why I haven't.

…ihs.from-platform-group

* 'ihs.from-platform-group' of github.com:wxtim/cylc: (75 commits)
  Update cylc/flow/task_job_mgr.py
  Update cylc/flow/exceptions.py
  Add unit test.
  update cfgspec doc
  Cycle point format colon restriction
  Codecov: use flags for fast & functional tests
  Remove :cylc:conf:
  Attempt to infer run number if there is no runN symlink. (cylc#4373)
  Update cylc/flow/cfgspec/globalcfg.py
  Update cylc/flow/cfgspec/globalcfg.py
  Added ..attention:: to localhost == scheduler host NOT ROSE_ORIG_HOST
  Add `versionchanged` admonitions to certain config items
  Add selection to global.cylc[platforms][localhost] Prevents documentation build failure.
  GH Actions: Update action
  Fix test
  codecov: fiddle configuration
  Update changelog
  Change 'ignore' to 'reload' for cycle point cli opts
  Tidy
  Run the container after pulling the image from GHCR
  ...
…ihs.from-platform-group

* 'ihs.from-platform-group' of github.com:wxtim/cylc:
  GH Actions: Add test workflows badges to release PRs
  Hot fix line length.
  response to review.
  Apply suggestions from code review
  Apply suggestions from code review
  Update cylc/flow/cfgspec/globalcfg.py
  Added deprecation warnings in platforms section
  platform documentation changes
  cli: change list_plugins to return a string
  cli: remove gui from command blacklist
  schema: correct faulty enum
@wxtim wxtim requested a review from hjoliver September 1, 2021 10:25
@wxtim
Copy link
Member Author

wxtim commented Sep 1, 2021

@hjoliver

Could get coverage up a bit?

Probably not. As far as I can see the failed lines are all covered by /tests/functional/intelligent_host_selection/06...
I could always add a coverage ignore.

And now it says patch is 100%. I'm confused.

@MetRonnie
Copy link
Member

MetRonnie commented Sep 1, 2021

Note that Codecov starts reporting coverage as soon as it receives the first uploaded report, so it will initially have the (low) fast-tests coverage only until the functional tests finish

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, need to do some heavy testing.

cylc/flow/exceptions.py Outdated Show resolved Hide resolved
cylc/flow/exceptions.py Outdated Show resolved Hide resolved
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(review in process...)

cylc/flow/cfgspec/globalcfg.py Show resolved Hide resolved
@wxtim wxtim requested a review from hjoliver September 21, 2021 14:38
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(Releasing some pending review comments from a few days ago...)

cylc/flow/platforms.py Outdated Show resolved Hide resolved
cylc/flow/platforms.py Outdated Show resolved Hide resolved
cylc/flow/platforms.py Outdated Show resolved Hide resolved
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Looks good, tests as working, nice 👍 No real problems found.

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
…latform-group

* 'master' of https://github.com/cylc/cylc: (101 commits)
  Update cylc/flow/cfgspec/workflow.py
  clean up interim obselete message
  fix flake8-simplify failures caused by recently added checks. (cylc#4420)
  Fix Change log
  Add test and raise error on src dir move
  change log
  Source symlink fix
  publish pending flag
  handle orphan tasks post reload
  Create window elements & load jobs from DB
  Deltas batched post reload
  Fix a doctest.
  Add colour to GH Actions pytest log
  Address review feedback.
  Type annotations for timer.
  Correct func test timeouts.
  Fix test.
  Add Cylc 7 back-compat func tests.
  Add stall restart func test.
  Func test for workflow timeout.
  ...
@wxtim
Copy link
Member Author

wxtim commented Sep 23, 2021

Strongly suggest that commits are squashed on merge.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

A few small docs comments.

cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved
cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved
cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved
cylc/flow/platforms.py Show resolved Hide resolved
Comment on lines +62 to +74
for host in {1..4}; do
named_grep_ok "job submit fails for bad_host${host}"\
"\"remote-init\" failed.*\"bad_host${host}\"" \
"$logfile"
done

# Look for message indicating that remote init has failed on both bad platforms
# in the platform group.
for platform in {1..2}; do
named_grep_ok "job submit fails for badplatform${platform}"\
"badhostplatform${platform}: Tried all the hosts"\
"$logfile"
done
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

🍾

@oliver-sanders oliver-sanders merged commit 1a1341c into cylc:master Oct 4, 2021
@wxtim wxtim deleted the ihs.from-platform-group branch October 5, 2021 07:53
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.

platforms: platform and host selection methods and intelligent fallback
4 participants