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

Add CLI http comms method #5267

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Dec 9, 2022

Partially address #5235
Sibling to cylc/cylc-uiserver#396

Description to come.

Check List

  • 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).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

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.

Nice.

We should be able to add the HTTPS comms method to the existing test battery. I've had a crack at the config here but haven't tried it out yet: https://github.com/oliver-sanders/cylc-flow/pull/new/swarm-https

Due to the way the test battery works there are two hosts:

  • The one where tests run:
    • This is either your own machine or the GitHub actions runner.
    • It has cylc-flow installed.
  • The job host:
    • This is the docker image, pretending to be another host.
    • It will have the UIS installed and the hub running.

So existing tests would try submitting jobs to the host which has the UIS running on it. This isn't how HTTPS comms would be used IRL as we would expect it to be installed on a third machine, however, this should be good enough for testing purposes. Jobs will contact the hub (on the same host) which will then TCP back to the test machine.

Any tests which define REQUIRE_PLATFORM that matches _remote_background_indep_https (e.g. * & tcp:*) would be run against HTTPS mode comms.

To write tests specifically for HTTPS comms use REQUIRE_PLATFORM='comms:https'. You can ssh $CYLC_TEST_HOST to test communication the more natural way around.

cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved
Comment on lines 1201 to 1209
http
Via the running Hub proxy and/or UI-Server (requires UI
Server installation)
Copy link
Member

Choose a reason for hiding this comment

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

How does cylc-flow know which port to contact the hub on when it doesn't share a filesystem with the hub? Do we need to configure this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the user might need to copy/replicate the ~/.cylc/uiserver/api_info.json file (if we don't figure out a way to automate it), because the server in question might not have ssh access (if just for CLI purposes)..

The other issue is, if the UIS gets restarted then the token will change.. Will having the scheduler monitor this be an issue? would we test with each job submission?

Copy link
Member

Choose a reason for hiding this comment

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

ATM clients are generally short lived (with the exception of the cylc tui client) so this isn't a biggie.

I guess reload this file if we get a comms failure and retry (once) if the file contents has changed.

@dwsutherland
Copy link
Member Author

We should be able to add the HTTPS comms method to the existing test battery. I've had a crack at the config here but haven't tried it out yet: https://github.com/oliver-sanders/cylc-flow/pull/new/swarm-https

Should we merge that branch into this?

Question:

  • Use http or https label?

HTTPS is just the secure version or HTTP but both are still know as HTTP, so given people can use HTTP versions of UIS/hub-proxy, I stuck with http... Or is the point to encourage S use?

@oliver-sanders
Copy link
Member

I would label it https just to avoid people thinking it might be unsecured.

cylc/flow/cfgspec/globalcfg.py Show resolved Hide resolved
cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved
cylc/flow/cfgspec/globalcfg.py Show resolved Hide resolved
@dwsutherland dwsutherland force-pushed the cli-via-uis branch 2 times, most recently from 3aeffc4 to 0a4231f Compare February 1, 2023 04:26
dwsutherland and others added 2 commits February 3, 2023 15:36
* Add a new docker image with cylc-uiserver/cylc-hub installed.
* Start the hub on a configured port.
@dwsutherland dwsutherland force-pushed the cli-via-uis branch 3 times, most recently from 7fb0fef to ffc423f Compare February 3, 2023 03:32
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.

None yet

2 participants