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

CreateOrReplace not fully equivalent with apply #3896

Closed
tacf opened this issue Feb 24, 2022 · 10 comments
Closed

CreateOrReplace not fully equivalent with apply #3896

tacf opened this issue Feb 24, 2022 · 10 comments

Comments

@tacf
Copy link

tacf commented Feb 24, 2022

Describe the bug

Client Version: 5.12.1
K8s version: 1.19

Function createOrReplace is not equivalent to kubectl apply is instead behaves more like kubectl create (as expressed in the code comments )

Despite being an old comment this is what we're experiencing (kubernetes/kubernetes#25238 (comment))

what is currently happening is that we have

  • Deployment with no replica set
  • HPA with min replicas 1

when using manually kubectl apply it behaves as expected, doing the proper rolling update (25%, max unavailable 0)

when using the client it behaves like a create(or replace) operation forcing the deployment object to update to replicas 1 (default by k8s) and updating the replica set back to 1 this immediately terminates all remaining pods breaking what would else be a smooth deployment.

This completely invalidates the use of fabric8 client for applying manifests in a safe way.

Other refs:
kubernetes/kubernetes#25238 (comment)
https://www.linkedin.com/pulse/kubectl-createreplace-vs-apply-which-one-use-samrat-sen/

Fabric8 Kubernetes Client version

5.12.1@latest

Steps to reproduce

  1. Use client apply twice a manifest with
    1.1 HPA 10 min replicas (10 just allows to easily observe the behaviour)
    1.2 Deployment with no replicas specified, rolling update (max surge 25%, max unavailable 0)

Expected behavior

Second apply should show a smooth transition of the pods to the new ones.

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

other (please specify in additional context)

Environment

Amazon

@shawkins
Copy link
Contributor

Related to #3420 #2060 #3566

The simplest change that would get much closer to an apply would be instead of a replace doing a json patch.

@shawkins
Copy link
Contributor

@manusa @rohanKanojia I'm not sure what all of the rationale behind not supporting apply were in the past. From what I can guess the logic to do this looks approximately like:

T apply(T modified) {
  Resource<T> resource = client.resource(modified);
  try {
    // not null safe, and should clone modified.  Also should use modified serialization that uses ObjectMetaMixIn  
    modified.getAnnotations().set("kubectl.kubernetes.io/last-applied-configuration", Serialization.asYaml(modified));
    return resource.create(modified);
  } catch (KubernetesClientException e) {
    // check e for conflict

    T base = resource.fromServer().get();
    if (base == null) {
       // retry ?
    }

    // not null safe
    String lastApplied = base.getAnnotations().get("kubectl.kubernetes.io/last-applied-configuration");
    if (lastApplied == null) {
      // merge patch?
    } else {
      T base = Serialization.unmarshall(lastApplied);
      // defaults to a json patch based upon a diff between base and modified
      return return client.resource(base).patch(modified);
    }
  }
} 

Obviously there's a lot of refinement needed and it's questionable whether we should reuse the same apply annotation, but it seems possible that we can offer a sensible alternative pretty quickly. What has come up previously as to why we haven't pursued a solution?

@shawkins
Copy link
Contributor

Linking to #3334 as well for server side apply support. More than likely we don't even have to add the last-applied-configuration and can just rely on issuing patches as application/apply-patch+yaml

@shawkins
Copy link
Contributor

shawkins commented Mar 8, 2022

See #3334 (comment) for 6.0 server side apply support. Additional apply support will be considered for 6.x.

@sunix
Copy link
Collaborator

sunix commented Mar 14, 2022

@tacf is it ok to close this issue ? as it is covered in #3334

@tacf
Copy link
Author

tacf commented Mar 14, 2022

Yes. I'll follow the mentioned one. Tyvm

@shawkins
Copy link
Contributor

@sunix @rohanKanojia it would probably be good to still have an issue for a higher-level apply or serverSideApply method. That will be easier for users to find.

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label Jun 12, 2022
@stale stale bot closed this as completed Jun 19, 2022
@manusa manusa reopened this Sep 1, 2022
@sunix
Copy link
Collaborator

sunix commented Sep 1, 2022

is this fixed with #3937 ?

@manusa
Copy link
Member

manusa commented Sep 1, 2022

is this fixed with #3937 ?

No, see #3896 (comment).

It would be nice to have a DSL method to perform the Server Side Apply logic, instead of manually configuring the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants