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 static IP jobs for assisted-service and assisted-test-infra #21604

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

osherdp
Copy link
Contributor

@osherdp osherdp commented Sep 5, 2021

We should have some basic job to test static IP configuration via REST API, as opposed to our kube-api job.
Some customers define static IP through our REST API endpoint for setting up nmstate configuration.
/cc @YuviGold @romfreiman

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2021

@osherdp: GitHub didn't allow me to request PR reviews from the following users: osherdp.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @osherdp
/hold

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.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 5, 2021
@osherdp
Copy link
Contributor Author

osherdp commented Sep 5, 2021

/test pj-rehearse

1 similar comment
@osherdp
Copy link
Contributor Author

osherdp commented Sep 5, 2021

/test pj-rehearse

@osherdp osherdp force-pushed the build/static-ips branch 2 times, most recently from 932ea66 to a76142f Compare September 5, 2021 11:25
@osherdp osherdp changed the title WIP add static IP presubmit job for assisted-service Sep 5, 2021
@osherdp
Copy link
Contributor Author

osherdp commented Sep 5, 2021

/cc @YuviGold @romfreiman

Copy link
Contributor

@flaper87 flaper87 left a comment

Choose a reason for hiding this comment

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

👍 for adding this job!

cluster_profile: packet
env:
ASSISTED_CONFIG: |
IS_STATIC_IP=true
Copy link
Contributor

@pawanpinjarkar pawanpinjarkar Sep 8, 2021

Choose a reason for hiding this comment

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

Can we rename the env var to signal to which code base it refers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which env-var? ASSISTED_CONFIG?

Copy link
Contributor

Choose a reason for hiding this comment

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

IS_STATIC_IP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's under ASSISTED_CONFIG
Also all other env-vars are not indicative about their source, NUM_MASTERS, IPV4, etc.

@flaper87
Copy link
Contributor

flaper87 commented Sep 8, 2021

@osherdp Can you elaborate on what this job will actually test? I'm digging a bit into test-infra and it looks like it will deploy a spoke cluster (I'm assuming SNO) with static IP (using NMStateConfig).

Any other details you can share? Is there a description field in the job definition where we can put this information in?

P.S: I'd love for our jobs definition to be easier to understand to people that are not familiar with the infrastructure itself

@osherdp
Copy link
Contributor Author

osherdp commented Sep 9, 2021

@osherdp Can you elaborate on what this job will actually test? I'm digging a bit into test-infra and it looks like it will deploy a spoke cluster (I'm assuming SNO) with static IP (using NMStateConfig).

Any other details you can share? Is there a description field in the job definition where we can put this information in?

P.S: I'd love for our jobs definition to be easier to understand to people that are not familiar with the infrastructure itself

Sure thing
It's pretty much doing what described in the PR description. We're deploying assisted in a simulating environment to our SaaS environment, and advancing installation using REST API. It's not using kube-api so the terminology of spoke-cluster and NMStateConfig might not be 100% accurate. Unless specifying otherwise, it should use 3 master nodes and 2 workers by default.

It doesn't seem like there's a description field for jobs unfortunately, as opposed to workflows and steps. We mainly follow the following pattern though that can explain most jobs:

<test-type>-<platform>-<flow>[-<variant>]

For example e2e-metal-assisted-ipv6, subsystem-aws-kubeapi or sometimes some jobs don't have variants and defaults apply: e2e-metal-assisted (3 masters and 2 workers, just ipv4 networking).

It seems like a lot of repos in openshift follow this pattern (or rather we follow their pattern), but maybe we can change the names on some places where it's really up to us. For example, find another name to distinct between assisted (SaaS, REST-operated) and assisted-operator (on-prem, kube-operated, ztp)

@osherdp osherdp changed the title add static IP presubmit job for assisted-service add static IP jobs for assisted-service and assisted-test-infra Sep 9, 2021
@osherdp osherdp force-pushed the build/static-ips branch 3 times, most recently from 0d4bf23 to 72e2141 Compare September 9, 2021 04:19
@YuviGold
Copy link
Contributor

YuviGold commented Sep 9, 2021

@osherdp Can you elaborate on what this job will actually test? I'm digging a bit into test-infra and it looks like it will deploy a spoke cluster (I'm assuming SNO) with static IP (using NMStateConfig).
Any other details you can share? Is there a description field in the job definition where we can put this information in?
P.S: I'd love for our jobs definition to be easier to understand to people that are not familiar with the infrastructure itself

Sure thing
It's pretty much doing what described in the PR description. We're deploying assisted in a simulating environment to our SaaS environment, and advancing installation using REST API. It's not using kube-api so the terminology of spoke-cluster and NMStateConfig might not be 100% accurate. Unless specifying otherwise, it should use 3 master nodes and 2 workers by default.

It doesn't seem like there's a description field for jobs unfortunately, as opposed to workflows and steps. We mainly follow the following pattern though that can explain most jobs:

<test-type>-<platform>-<flow>[-<variant>]

For example e2e-metal-assisted-ipv6, subsystem-aws-kubeapi or sometimes some jobs don't have variants and defaults apply: e2e-metal-assisted (3 masters and 2 workers, just ipv4 networking).

It seems like a lot of repos in openshift follow this pattern (or rather we follow their pattern), but maybe we can change the names on some places where it's really up to us. For example, find another name to distinct between assisted (SaaS, REST-operated) and assisted-operator (on-prem, kube-operated, ztp)

This is a very good explanation! I think we should have it in our docs as part of the CI documentation we were talking about

Copy link
Contributor

@YuviGold YuviGold left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@osherdp
Copy link
Contributor Author

osherdp commented Sep 9, 2021

@YuviGold we have decided where it should be hosted? :)

@osherdp
Copy link
Contributor Author

osherdp commented Sep 9, 2021

/test all

@osherdp
Copy link
Contributor Author

osherdp commented Sep 9, 2021

/refresh

@osherdp
Copy link
Contributor Author

osherdp commented Sep 9, 2021

ok, I think IS_STATIC_IP configuration doesn't work
I'll check with @eliorerz what happens here

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@osherdp
Copy link
Contributor Author

osherdp commented Sep 9, 2021

co-depends with openshift/assisted-test-infra#1099
(not really critical which one will get merged first)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

@osherdp: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/clusterimageset-updater 377caee link /test clusterimageset-updater

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-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: osherdp, YuviGold

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

@YuviGold
Copy link
Contributor

YuviGold commented Sep 9, 2021

/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 Sep 9, 2021
@openshift-merge-robot openshift-merge-robot merged commit f897f7a into openshift:master Sep 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

@osherdp: Updated the following 2 configmaps:

  • ci-operator-master-configs configmap in namespace ci at cluster app.ci using the following files:
    • key openshift-assisted-service-master.yaml using file ci-operator/config/openshift/assisted-service/openshift-assisted-service-master.yaml
    • key openshift-assisted-test-infra-master.yaml using file ci-operator/config/openshift/assisted-test-infra/openshift-assisted-test-infra-master.yaml
  • job-config-master configmap in namespace ci at cluster app.ci using the following files:
    • key openshift-assisted-service-master-presubmits.yaml using file ci-operator/jobs/openshift/assisted-service/openshift-assisted-service-master-presubmits.yaml
    • key openshift-assisted-test-infra-master-periodics.yaml using file ci-operator/jobs/openshift/assisted-test-infra/openshift-assisted-test-infra-master-periodics.yaml
    • key openshift-assisted-test-infra-master-presubmits.yaml using file ci-operator/jobs/openshift/assisted-test-infra/openshift-assisted-test-infra-master-presubmits.yaml

In response to this:

We should have some basic job to test static IP configuration via REST API, as opposed to our kube-api job.
Some customers define static IP through our REST API endpoint for setting up nmstate configuration.
/cc @YuviGold @romfreiman

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants