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

Bump to kubernetes-v1.18.0-beta.2 #116

Conversation

marun
Copy link

@marun marun commented Mar 13, 2020

marun and others added 2 commits March 12, 2020 00:44
…jq 'select(.Version!="v0.0.0")') > Godeps/Godeps.json
The tests need to be rewritten to be safe for concurrent use and for
work in contended environments. Disabling the worst offenders and fixing
reuse issues around the tests here.

Origin-commit: b6281a54c84f20c2f0d35d6a44881e83b2e75227
@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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 13, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marun
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found 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

@openshift-ci-robot openshift-ci-robot added the area/dependency Issues or PRs related to dependency changes label Mar 13, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Mar 13, 2020
@soltysh
Copy link
Member

soltysh commented Mar 13, 2020

A few comments for now:

  1. Squash all:
UPSTREAM: <carry>: Expose a simple journald shim on the kubelet logs endpoint 
UPSTREAM: <carry>: Expose a simple journald shim on the kubelet logs endpoint 
UPSTREAM: <carry>: Make default journal format precise

together, it's all about exposing that journald through kubelet, so it's sufficient to have a single commit.

  1. It would be reasonable to squash kcm related carries into a single one I mean these:
UPSTREAM: <carry>: GC doesn't ignore hpa and securitycontextconstraints 
UPSTREAM: <carry>: GC doesn't ignore apps/deployments 
UPSTREAM: <carry>: switch to use external API
UPSTREAM: <carry>: scheduling: Node selector aware DS controller should not process openshift-io/node-selector if scheduler.alpha.kubernetes.io/node-selector is set.
UPSTREAM: <carry>: kube-controller-manager: allow running bare kube-controller-manager 

@marun
Copy link
Author

marun commented Mar 13, 2020

A few comments for now:

  1. Squash all:
UPSTREAM: <carry>: Expose a simple journald shim on the kubelet logs endpoint 
UPSTREAM: <carry>: Expose a simple journald shim on the kubelet logs endpoint 
UPSTREAM: <carry>: Make default journal format precise

Done

  1. It would be reasonable to squash kcm related carries into a single one I mean these:
UPSTREAM: <carry>: GC doesn't ignore hpa and securitycontextconstraints 
UPSTREAM: <carry>: GC doesn't ignore apps/deployments 
UPSTREAM: <carry>: switch to use external API
UPSTREAM: <carry>: scheduling: Node selector aware DS controller should not process openshift-io/node-selector if scheduler.alpha.kubernetes.io/node-selector is set.
UPSTREAM: <carry>: kube-controller-manager: allow running bare kube-controller-manager 

Done. Are there any more that kcm carries that should be squashed? i.e.

UPSTREAM: <carry>: allow running bare kube-controller-manager
UPSTREAM: <carry>: kube-controller-manager: exclude some origin resources from quota
UPSTREAM: <carry>: kube-controller-manager: add service serving cert signer to token controller

@marun marun force-pushed the origin-4.5-kubernetes-v1.18.0-beta.2 branch 2 times, most recently from e420c8c to 6992c9a Compare March 16, 2020 06:40
@marun
Copy link
Author

marun commented Mar 16, 2020

Unit tests are passing locally. Commits past 7a4ff00 are new fixes rather than picks. Is make verify supposed to be passing?

{Group: "authorization.openshift.io", Resource: "clusterroles"}: {},
{Group: "authorization.openshift.io", Resource: "clusterrolebindings"}: {},
{Group: "apiregistration.k8s.io", Resource: "apiservices"}: {},
{Group: "apiextensions.k8s.io", Resource: "customresourcedefinitions"}: {},
Copy link

Choose a reason for hiding this comment

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

@deads2k why do we exclude apiservers and CRDs?

Copy link

Choose a reason for hiding this comment

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

@deads2k why do we exclude apiservers and CRDs?

I have no memory. They are cluster scoped and not covered by quota. Should be removeable.

Copy link
Author

Choose a reason for hiding this comment

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

As per a slack conversation, we can drop cluster-scoped resources from this list.

Copy link
Author

Choose a reason for hiding this comment

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

But trimming the file is deferred till after this lands to not delay things further.

// Get Volume
volume, err := os.getVolume(pv.Spec.Cinder.VolumeID)
// Get metadata
md, err := getMetadata(os.metadataOpts.SearchOrder)
Copy link

Choose a reason for hiding this comment

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

@Fedosin why is this a carry and not upstreamed?

Copy link
Author

Choose a reason for hiding this comment

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

Given that @Fedosin appears to be on vacation, I think this concern can be deferred for now. I've prompted him on slack so hopefully he answers when he returns.

Copy link

Choose a reason for hiding this comment

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

Hi! I did this because there was a requirement to read cloud credentials from a secret. The upstream version doesn't support it, as you know.
As I remember, there was an issue with kube-apiserver to get a volume info, so I implemented this workaround.

I believe we can revert it now and go back to the upstream implementation.

p0lyn0mial and others added 23 commits March 17, 2020 11:08
Origin-commit: a1d5a1201d5fb4616187cd009f126f5fe7fa0787
Origin-commit: beac12d815b4099cfd4f4d953da4b8789054be51
…ofile

Origin-commit: 84ba7fc304870a30df7136da14bccb4d5232f075
Origin-commit: 4498bb4de03ff3a910fed10bed337ba2fcdf321d
This line is not necessary for our test usage and should not be an
issue in OpenShift (openshift-tests already verifies this correctly).
…y running test

OpenShift uses these function before any test is run and they cause NPE
Origin-commit: 131dbb4770bb3bed0c07d2a6ca0cbe4cba2556bb
This line makes the upgrade log output unreadable and provides
no value during the set of tests it's used in:

```
Jan 12 20:49:25.628: INFO: cluster upgrade is Progressing: Working towards registry.svc.ci.openshift.org/ci-op-jbtg7jjb/release@sha256:144e73d125cce620bdf099be9a85225ade489a95622a70075d264ea3ff79219c: downloading update
Jan 12 20:49:26.692: INFO: Poke("http://a74e3476115ce4d2d817a1e5ea608dad-802917831.us-east-1.elb.amazonaws.com:80/echo?msg=hello"): success
Jan 12 20:49:28.727: INFO: Poke("http://a74e3476115ce4d2d817a1e5ea608dad-802917831.us-east-1.elb.amazonaws.com:80/echo?msg=hello"): success
```

Origin-commit: 1cdf04c0e15b79fad3e3a6ba896ed2bb3df42b78
…o function

Origin-commit: 0d7fb2d769d631054ec9ac0721aee623c96c1001
Origin-commit: cb0b340d0e68c9524fa7fd6277f571b6aa68bf86
The following packages have tests that exceed the default 120s
timeout:

k8s.io/kubernetes/pkg/kubelet/volumemanager/reconciler

 - The tests in this package collectively take longer than 120s.

k8s.io/kubernetes/pkg/volume/csi

 - One of the unit tests has to wait 2 minutes for a timeout to
   validate its failure condition.
@marun marun force-pushed the origin-4.5-kubernetes-v1.18.0-beta.2 branch from a324756 to 2dceee3 Compare March 17, 2020 18:15
@marun
Copy link
Author

marun commented Mar 17, 2020

Updated to remove OWNERS files, which were all from UPSTREAM: <carry>: openshift-kube-apiserver: add kube-apiserver patches.

@ironcladlou
Copy link

I'd like to help review this, but as someone who hasn't done a rebase in literally years, I don't really know where to begin. How does one evaluate whether the proposed tree is in sync with the upstream tag?

What procedure would you recommend to someone looking at this "from the outside" to be of any assistance? Apologies if there's some guide out there I'm missing. docs/rebase.md didn't really help.

@openshift-ci-robot
Copy link

@marun: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/unit 2dceee3 link /test unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@marun
Copy link
Author

marun commented Mar 17, 2020

Closing in favor of #118, which picks the same commits against rc.1

@marun marun closed this Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependency Issues or PRs related to dependency changes size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet