Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

use server side apply#4648

Merged
openshift-merge-robot merged 9 commits into
redhat-developer:mainfrom
kadel:ssa
May 17, 2021
Merged

use server side apply#4648
openshift-merge-robot merged 9 commits into
redhat-developer:mainfrom
kadel:ssa

Conversation

@kadel
Copy link
Copy Markdown
Member

@kadel kadel commented Apr 21, 2021

What type of PR is this?

/kind cleanup
/kind code-refactoring

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #4553

** Notes ***
There is a fallback for k8s < v1.16.0. On later versions, it falls back to not using server-side apply. This is ok, as the main reason for using SSA is SBO and SBO won't work on those k8s versions anyway.

I have executed ▶ make test-integration-devfile against ocp 3.11.
Two tests are failing.

Summarizing 2 Failures:

[Fail] odo devfile app command tests Testing URLs for OpenShift specific scenarios [It] it should list, describe and delete the apps 
/home/tomas/Code/Work/odo/tests/helper/helper_run.go:34

[Fail] odo devfile push command tests push with listing the devfile component [It] checks devfile and s2i components together 
/home/tomas/Code/Work/odo/tests/helper/helper_run.go:34

Ran 203 of 208 Specs in 1338.623 seconds
FAIL! -- 201 Passed | 2 Failed | 0 Pending | 5 Skipped

But the same two tests are failing on master as well. So this is not something that was introduced in this PR.

#4678

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/cleanup labels Apr 21, 2021
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 21, 2021
@dharmit dharmit mentioned this pull request Apr 21, 2021
4 tasks
@kadel kadel force-pushed the ssa branch 2 times, most recently from 1d833b3 to 2f9fca0 Compare April 26, 2021 12:29
@kadel kadel changed the base branch from master to main April 27, 2021 07:51
@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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

The pull request process is described here

Details 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

@kadel
Copy link
Copy Markdown
Member Author

kadel commented Apr 28, 2021

error: error creating buildah builder: Error reading signatures: Error downloading signatures for sha256:7157e96d3be123ef879a2c7061271a97b75d58b2669c9afc5ca35cebaa1ba720 in registry.build01.ci.openshift.org/ci/managed-clonerefs: received unexpected HTTP status: 504 Gateway Time-out

/retest

@kadel kadel changed the title [WIP] use server side apply use server side apply Apr 28, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Apr 28, 2021
@kadel kadel force-pushed the ssa branch 3 times, most recently from a4864ee to c28be78 Compare April 30, 2021 13:44
Comment thread pkg/kclient/deployments.go
Comment thread pkg/kclient/kclient.go
Comment thread pkg/auth/login.go
Comment thread pkg/kclient/deployments.go Outdated
Comment thread pkg/kclient/deployments.go Outdated
Comment thread pkg/kclient/kclient.go
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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

The pull request process is described here

Details 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

@dharmit
Copy link
Copy Markdown
Member

dharmit commented May 10, 2021

@kadel, First off, I know that I myself don't entertain the "what about users who modify devfile by hand" discussions. So I'm not asking to tackle the scenario here. I'm just putting my thoughts, and we can very well handle it at a later stage.

With the disclaimer in mind, should something change with regard to our approach for odo service create here: https://github.com/openshift/odo/blob/a920cccb8513da7fc012e699be3c8dda77e3d3ba/pkg/service/service.go#L856-L867

When I wrote that, I simply made sure to ignore the error. But if a user modified the service info (Kubernetes inlined thing) in devfile by hand, odo push won't be able to make changes on the cluster. My understanding/assumption/expectation was the with SSA, it would be possible for odo to handle such changes when doing subsequent odo push.

For example, I'm spinning up an EtcdCluster using below manifest:

- kubernetes:
    inlined: |
      apiVersion: etcd.database.coreos.com/v1beta2
      kind: EtcdCluster
      metadata:
        annotations:
          etcd.database.coreos.com/scope: clusterwide
        name: example
      spec:
        size: 3
        version: 3.2.13
  name: example

I then modified size from size: 3 to size: 1. Should odo push (with support for SSA) be able to scale down the pods for EtcdCluster?

@kadel
Copy link
Copy Markdown
Member Author

kadel commented May 10, 2021

I then modified size from size: 3 to size: 1. Should odo push (with support for SSA) be able to scale down the pods for EtcdCluster?

yes, it should. SSA is not required for this. It should update the existing resources.

@kadel
Copy link
Copy Markdown
Member Author

kadel commented May 10, 2021

/retest

Comment thread pkg/devfile/adapters/kubernetes/component/adapter.go
@kadel
Copy link
Copy Markdown
Member Author

kadel commented May 12, 2021

/retest

@kadel kadel force-pushed the ssa branch 2 times, most recently from 1b9daef to c28be78 Compare May 12, 2021 14:42
@kadel
Copy link
Copy Markdown
Member Author

kadel commented May 13, 2021

done, managed to a resolved failing test. It was due to conflicts with #4671

@feloy
Copy link
Copy Markdown
Contributor

feloy commented May 14, 2021

/lgtm

@feloy
Copy link
Copy Markdown
Contributor

feloy commented May 14, 2021

/hold

@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. Required by Prow. label May 14, 2021
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 14, 2021
@dharmit
Copy link
Copy Markdown
Member

dharmit commented May 17, 2021

Looks good to me.

/hold cancel

@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. Required by Prow. label May 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit 89ac67e into redhat-developer:main May 17, 2021
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/refactoring Issues or PRs related to code refactoring lgtm Indicates that a PR is ready to be merged. Required by Prow.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement server side apply

6 participants