kubernetes-client icon indicating copy to clipboard operation
kubernetes-client copied to clipboard

CreateOrReplace not fully equivalent with `apply`

Open tacf opened this issue 3 years ago • 10 comments

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 (https://github.com/kubernetes/kubernetes/issues/25238#issuecomment-306549174)

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: https://github.com/kubernetes/kubernetes/issues/25238#issuecomment-406415297 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

tacf avatar Feb 24 '22 15:02 tacf

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 avatar Feb 24 '22 22:02 shawkins

@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 avatar Feb 28 '22 16:02 shawkins

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 avatar Feb 28 '22 18:02 shawkins

See https://github.com/fabric8io/kubernetes-client/issues/3334#issuecomment-1062076433 for 6.0 server side apply support. Additional apply support will be considered for 6.x.

shawkins avatar Mar 08 '22 18:03 shawkins

@tacf is it ok to close this issue ? as it is covered in https://github.com/fabric8io/kubernetes-client/issues/3334

sunix avatar Mar 14 '22 11:03 sunix

Yes. I'll follow the mentioned one. Tyvm

tacf avatar Mar 14 '22 11:03 tacf

@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.

shawkins avatar Mar 14 '22 12:03 shawkins

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[bot] avatar Jun 12 '22 14:06 stale[bot]

is this fixed with https://github.com/fabric8io/kubernetes-client/pull/3937 ?

sunix avatar Sep 01 '22 08:09 sunix

is this fixed with #3937 ?

No, see https://github.com/fabric8io/kubernetes-client/issues/3896#issuecomment-1066739461.

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

manusa avatar Sep 01 '22 08:09 manusa