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.

Sign up for GitHub

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

Jump to bottom

BUG: client.UpdateStatus.Update() func clears Kind of the local object #406

Closed
TenSt opened this issue Apr 24, 2019 · 8 comments · Fixed by #528
Closed

BUG: client.UpdateStatus.Update() func clears Kind of the local object #406

TenSt opened this issue Apr 24, 2019 · 8 comments · Fixed by #528
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@TenSt
Copy link

TenSt commented Apr 24, 2019

Description
Using client.UpdateStatus.Update() func clears Kind of the local object.

How to reproduce
I've encountered this during my work with operator SDK. So repro steps use it too:

  1. Get instance using:
    r.client.Get(context.TODO(), request.NamespacedName, instance)
  2. Update status of the CR:
    instance.Status.Result = "success"
  3. Run client.Status().Update() func to update object in the K8S:
    'r.client.Status().Update(context.TODO(), instance)'
  4. Look at the Kind fields in the instance object:
    reqLogger.Info("Instance typeMeta kind: " + instance.TypeMeta.Kind) reqLogger.Info("Instance kind: " + instance.Kind)

Actual result
client.Status().Update() func cleared Kind fields thus instance.TypeMeta.Kind and instance.Kind are empty.

Expected result
client.Status().Update() func did nothing with Kind fields thus instance.TypeMeta.Kind and instance.Kind store appropriate values.

Additional information
Here is a link to my repo with simple operator which demonstrates this behavior.
https://github.com/TenSt/app-operator

Please let me know if you need any additional information.
@DirectXMan12
Copy link
Contributor

/kind bug
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels May 4, 2019
@GrigoriyMikhalkin
Copy link
Contributor

GrigoriyMikhalkin commented May 12, 2019 •
edited
Loading

Bug is caused by kubernetes/apimachinery#19. Code comment implying that this is desired behavior...

@jcrossley3
Copy link

I'm just now hitting this, too. Any thoughts on a workaround?

@GrigoriyMikhalkin
Copy link
Contributor

@jcrossley3 Not sure, but it seems that you have to write your own implementation of Client interface for that. At least i didn't found any other way to pass your own decoder to client.New function, for now.

@jcrossley3
Copy link

@DirectXMan12 is this too naive of a fix? https://gist.github.com/jcrossley3/16621ee70f21c095c11be1262ad04ee2

If not, I can PR.

@DirectXMan12
Copy link
Contributor

I think that actually looks good. I'd have to double check where we do it for other things, but that's probably fine.

@jcrossley3
Copy link

I tried to write a failing test for it, but it seems it's not broken in whatever harness is used to run the tests. Did it accidentally get fixed? :)

@DirectXMan12
Copy link
Contributor

The test harness just uses the kube apiserver. You can check the version used in hack/check-everything.sh. It's 1.14.1 on CR master

@negz negz mentioned this issue Jun 12, 2019
Merged
9 tasks
@vincepri vincepri mentioned this issue Jul 19, 2019
Closed
sebgl added a commit to sebgl/cloud-on-k8s that referenced this issue Jul 22, 2019
@sebgl
Don't rely on buggy metaObject Kind
d8c90f4 
A bug in our client implementation may clear the object's Kind on
certain scenarios. See
kubernetes-sigs/controller-runtime#406.

Let's avoid that by fixing a constant Kind returned by a method call on
the resource.
@sebgl sebgl mentioned this issue Jul 22, 2019
Merged
@vincepri vincepri mentioned this issue Jul 22, 2019
Merged
sebgl added a commit to elastic/cloud-on-k8s that referenced this issue Jul 24, 2019
@sebgl
Don't rely on buggy metaObject Kind (#1324)
51c4be8 
* Don't rely on buggy metaObject Kind

A bug in our client implementation may clear the object's Kind on
certain scenarios. See
kubernetes-sigs/controller-runtime#406.

Let's avoid that by fixing a constant Kind returned by a method call on
the resource.
sebgl added a commit to sebgl/cloud-on-k8s that referenced this issue Jul 24, 2019
@sebgl
Don't rely on buggy metaObject Kind (elastic#1324)
8fd649d 
* Don't rely on buggy metaObject Kind

A bug in our client implementation may clear the object's Kind on
certain scenarios. See
kubernetes-sigs/controller-runtime#406.

Let's avoid that by fixing a constant Kind returned by a method call on
the resource.
sebgl added a commit to elastic/cloud-on-k8s that referenced this issue Jul 24, 2019
@sebgl
Don't rely on buggy metaObject Kind (#1324) (#1357)
36713c6 
* Don't rely on buggy metaObject Kind

A bug in our client implementation may clear the object's Kind on
certain scenarios. See
kubernetes-sigs/controller-runtime#406.

Let's avoid that by fixing a constant Kind returned by a method call on
the resource.
sebgl added a commit to elastic/cloud-on-k8s that referenced this issue Jul 24, 2019
@sebgl
Merge master into statefulset-refactoring (#1358)
8b75c62 
* Use the setvmmaxmapcount initcontainer by default in E2E tests (#1300)

Let's keep our default defaults :)

The setting is disabled explicitly for E2E tests where we enable a
restricted security context.

* Add docs for plugins, custom configuration files and secure settings (#1298)

* Allow license secret webhook to fail (#1301)

Webhooks on core k8s objects are just too debilitating in case our
webhook service fails. This sets the failure policy for the secret
webhook to ignore to strike a balance between UX (immediate feedback)
and keeping the users k8s cluster in a working state. Also we have an
additional validation run on controller level so this does not allow
circumventing our validation logic.

* Revert "Use the setvmmaxmapcount initcontainer by default in E2E tests (#1300)" (#1302)

This reverts commit fff1526.
This commit is breaking our E2E tests chain, which deploy a
PodSecurityPolicy by default. Any privileged init container will not
work.

I'll open an issue for a longer-term fix to properly handle this.

* Update quickstart (#1307)

* Update the name of the secret for the elastic user
* Bump the Elastic Stack version from 7.1.0 to 7.2.0

* Change Kibana readiness endpoint to return a 200 OK (#1309)

The previous endpoint returned an http code 302. While this is fine for
Kubernetes, some derived systems like GCP LoadBalancers mimic the
container readiness check for their own readiness check. Except GCP
Loadbalancers only work with status 200.

It's not up to us to adapt GCP LoadBalancers to K8s, but this is a
fairly trivial fix.

* Fix pod_forwarder to support two part DNS names, adjust e2e http_client (#1297)

* Fix pod_forwarder to support two part DNS names, adjust e2e http_client url

* Revert removing .svc in e2e http_client

* [DOC] Resources management and volume claim template (#1252)

* Add resources and persistent volume templates documentation

* Ignore resources reconciled by older controllers (#1286)

* Document PodDisruptionBudget section of the ES spec (#1306)

* Document PodDisruptionBudget section of the ES spec

I suspect this might slightly change in the feature depending on how we
handle the readiness check, so I'm keeping this doc minimal for now:

* what is a PDB, briefly (with a link)
* default PDB we apply
* how to set a different PDB
* how to disable the default PDB

* Move version out from Makefile (#1312)

* Add release note generation tool (#1314)

* no external dependencies
* inspects PRs by version label
* generates structured release notes in asciidoc grouped by type label

* Add console output to standalone apm sample (#1321)

* Update Quickstart to 0.9.0 (#1317)

* Update doc (#1319)

* Update persistent storage section
* Update kibana localhost url to use https
* Update k8s resources names in accessing-services doc
* Mention SSL browser warning
* Fix bulleted list

* Add CI job for nightly builds (#1248)

* Move version to a file

* Add CI implementation

* Update VERSION

* Depend on another PR for moving out version from Makefile

* Update Jenkinsfile

* Don't build and push operator image in bootstrap-gke (#1332)

We don't need to do that anymore, since we don't use an init container
based on the operator image.

* Remove Docker image publishing from devops-ci (#1339)

* Suppress output of certain commands from Makefile (#1342)

* Document how to disable TLS (#1341)

* Use new credentials for Docker registry (#1346)

* Workaround controller-runtime webhook upsert bug (#1337)

* Fix docs build on PR job (#1351)

* Fix docs build on PR job

* Cleanup workspace before doing other steps

* APM: remove "output" element and add elasticsearchRef (#1345)

* Don't rely on buggy metaObject Kind (#1324)

* Don't rely on buggy metaObject Kind

A bug in our client implementation may clear the object's Kind on
certain scenarios. See
kubernetes-sigs/controller-runtime#406.

Let's avoid that by fixing a constant Kind returned by a method call on
the resource.
@anyasabo anyasabo mentioned this issue Oct 1, 2019
Closed
@racevedoo racevedoo mentioned this issue Oct 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🐛 Preserve GroupVersionKind during Update/Patch vincepri/controller-runtime
5 participants
@jcrossley3 @DirectXMan12 @GrigoriyMikhalkin @k8s-ci-robot @TenSt