kubernetes: Core e2e test framework should not import sub e2e test frameworks

@mauriciopoppe First, thanks for the well written proposal!

Glossary:

• f/ = test/e2e/framework/
• f/pod = test/e2e/framework/pod

Plan:

• Move all of the functions of f/pod.go (PodClient) to f/pod/ inside another compat module e.g. e2epodcompat
• Move all of the functions of f/utils.go to f/pod/, this one is easier and can be moved to f/pod as it is
• Refactor the usage of these functions to the new signature e2epodcompat.Create, e2epodcompat.Update, etc
• Match e2epodcompat.Create with existing methods inside e2epod, eventually remove e2epodcompat

This sounds reasonable to me. My concern is potential code/rebase churn this may cause during the runup to code freeze in the next week.

# from kubernetes/kubernetes/test
$ rg -l 'PodClient()' | xargs -n1 dirname | sort | uniq -c
   4 e2e/apps
   1 e2e/auth
  13 e2e/common
   1 e2e/common/node
   7 e2e/common/storage
   3 e2e/framework
   1 e2e/framework/network
   1 e2e/framework/security
   1 e2e/instrumentation/logging/utils
   1 e2e/instrumentation/monitoring
   6 e2e/network
   5 e2e/node
   1 e2e/scheduling
   2 e2e/storage
   2 e2e/upgrades
  10 e2e/windows
  32 e2e_node

So one suggestion I have is: go with the above plan, but don’t land anything that’s going to cause a lot of in-flight PR’s to need to rebase. After code freeze, go nuts, but need to complete prior to test freeze, and be prepared to back off if it’s too disruptive.

---

Another suggestion would be to try hiding PodClient’s framework dependency behind a shim, e.g.

Modify Framework to expose some things via functions instead of direct struct access (this isn’t very golang, but bear with me)

// f/framework.go
func (f *Framework) GetNamespace() *v1.Namespace {
  return f.Namespace
}
func (f *Framework) GetClientSet() clientset.Interface {
  return f.ClientSet
}
// I'm not sure how I feel about this part, purely for the shim
// but others might start using it (and may need ExpectWithOffset tweak)
func (f *Framework) ExpectNoError(err error, explain ...interface{}) {
  ExpectNoError(err error, explain ...interface{})
}

Use the shim instead of framework to implement e2epod.PodClient

// f/pod/client.go
type FrameworkShim interface {
  GetNamepace() *v1.Namespace
  GetClientSet() clientset.Interface
  ExpectNoError(err, error, explain ...inteface{})
  // etc...
}
type PodClient struct {
  f FrameworkShim
  v1core.PodInterface
}
func NewPodClient(f FrameworkShim) *PodClient {
  return &PodClient{
    f:            f,
    PodInterface: f.GetClientSet().CoreV1().Pods(f.GetNamespace().Name),
  }
} 

// sample implementation 
func (c *PodClient) CreateSync(pod *v1.Pod) *v1.Pod {
  namespace := c.f.GetNamespace().Name
  p := c.Create(pod)
  c.f.ExpectNoError(e2epod.WaitTimeoutForPodReadyInNamespace(c.f.GetClientSet(), p.Name, namespace, PodStartTimeout))
  // Get the newest pod after it becomes running and ready, some status may change after pod created, such as pod ip.
  p, err := c.Get(context.TODO(), p.Name, metav1.GetOptions{})
  c.f.ExpectNoError(err)
  return p
} 
// etc...

Then progressively migrate tests to use the new client instead of the old one

// f/foo/foo_test.go
import e2epod "k8s.io/kubernetes/test/framework/pod"

// migrate from code using the old client
var oldClient *framework.PodClient = f.PodClient()
podOld := f.PodClient().CreateSync(pod)

// to code using the new client
var newClient *e2epod.PodClient = e2epod.PodClient(f)
podNew := e2epod.PodClient(f).CreateSync(pod)

I’m not 100% sure the shim approach lacks tangles? But I think it changes fewer function signatures, and could be landed without forcing a ton of code to migrate at once.