fix(kuma-cp) logging of resource conflicts #1254

lobkovilya · 2020-12-02T05:07:35Z

Signed-off-by: Ilya Lobkov ilya.lobkov@konghq.com

Summary

Since we are using a caching client on Kubernetes Get might return an old version of the resource and cause Resource conflict error. Despite these errors in the log Kuma CP reconciles resources as expected. So current PR just eliminates such log lines for insight resyncer and vip allocator

Documentation

Not required

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jakubdyszkiewicz · 2020-12-02T13:14:11Z

pkg/dns/vips_allocator.go

+		if err != nil {
+			// Error might occur only if we run out of VIPs. There is no point to pass it through,
+			// we must notify user in logs and proceed
+			vipsAllocatorLog.Error(err, "failed to allocate new VIPs")


shouldn't we return after this one?

No, UpdateMeshedVIPs could allocate some IPs and we should update them in the configmap

jakubdyszkiewicz · 2020-12-02T13:15:53Z

pkg/plugins/runtime/k8s/controllers/configmap_controller.go

@@ -42,6 +43,10 @@ func (r *ConfigMapReconciler) Reconcile(req kube_ctrl.Request) (kube_ctrl.Result
 	}

 	if err := r.VIPsAllocator.CreateOrUpdateVIPConfig(mesh); err != nil {
+		if kube_apierrs.IsConflict(err) {


I believe this should be store.IsResourceConflict. We are translating Kube errors

Yes, but ConfigStore didn't handle it and returned an original error. Fixed both ConfigStore and configmap_controller

jakubdyszkiewicz · 2020-12-02T13:20:58Z

pkg/plugins/runtime/k8s/controllers/configmap_controller.go

@@ -42,6 +43,10 @@ func (r *ConfigMapReconciler) Reconcile(req kube_ctrl.Request) (kube_ctrl.Result
 	}

 	if err := r.VIPsAllocator.CreateOrUpdateVIPConfig(mesh); err != nil {
+		if kube_apierrs.IsConflict(err) {
+			r.Log.V(1).Info("resource conflict", "err", err)


sounds scary. This should be more descriptive like

r.Log.V(1).Info("VIPs were updated in the other place. Retrying")

also on V1 "updating VIPs for mesh" and after the operation "VIPs for mesh udpated

Agree, fixed these

jakubdyszkiewicz · 2020-12-02T13:21:18Z

pkg/core/resources/store/store.go

@@ -124,6 +124,10 @@ func ErrorResourceConflict(rt model.ResourceType, name, mesh string) error {
 	return fmt.Errorf("Resource conflict: type=%q name=%q mesh=%q", rt, name, mesh)
 }

+func IsResourceConflict(err error) bool {
+	return err != nil && strings.HasPrefix(err.Error(), "Resource conflict")
+}


merge origin/master, this function should be there

jakubdyszkiewicz · 2020-12-02T13:22:11Z

pkg/dns/vips_allocator.go

-func (d *VIPsAllocator) createOrUpdateVIPConfigs(meshes ...string) (errs error) {
+// createOrUpdateVIPConfigs updates VIP configs for provided meshes.
+// - fastFail - if 'true' return the first occurred error, stop processing meshes.
+//              if 'false' then process all meshes, return the list of errors


I see what failFast does, but I don't understand why 🤔 can you help me understand?

Just forget, I got rid of it

pkg/insights/resyncer.go

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jakubdyszkiewicz · 2020-12-10T10:34:59Z

pkg/xds/server/components.go

+						return reconciler.Clear(&proxyID)
+					}
+					return err
+				}


I don't think this fix the situation you described, because you used ReadOnlyResourceManager.
When you get the dataplane in the first step, it is cached so it's very high chance you will still retrieve V1. I think you need to either

Use non cached ResourceManager

Use the dataplane that was used to compiled the hash

This should be created as a separate PR imho

If you get Dataplane after calculating hash it is the same or newer than a dataplane which reflected in the hash. Because when calculating hash we are also using ReadOnlyResourceManager

@jakubdyszkiewicz create separate PR #1313, let's proceed there

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # api/go.sum # go.sum # pkg/plugins/resources/k8s/native/go.sum # pkg/plugins/runtime/k8s/controllers/configmap_controller.go

jakubdyszkiewicz · 2020-12-23T12:16:55Z

pkg/dns/vips_allocator_test.go

+
+		err = errAllocator.CreateOrUpdateVIPConfigs()
+		Expect(err).To(HaveOccurred())
+		fmt.Println(err)


jakubdyszkiewicz · 2020-12-23T12:17:29Z

pkg/dns/vips_allocator_test.go

+		err = errAllocator.CreateOrUpdateVIPConfigs()
+		Expect(err).To(HaveOccurred())
+		fmt.Println(err)
+		Expect(err.Error()).To(Equal("error during update, mesh = mesh-1; error during update, mesh = mesh-2"))


nit: Expect(err).To(MatchError(""))

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

(cherry picked from commit b348fb3)

(cherry picked from commit b348fb3) Co-authored-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) logging of resource conflicts

0f93e76

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya requested a review from a team as a code owner December 2, 2020 05:07

lobkovilya added the backport-to-stable label Dec 2, 2020

jakubdyszkiewicz reviewed Dec 2, 2020

View reviewed changes

lobkovilya added 4 commits December 3, 2020 23:34

fix(kuma-cp) log lines and bug with hash

39de090

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Merge branch 'master' into fix/resource-conflict-error

e2b25c6

fix(kuma-cp) review

7b6e5a9

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) make check

436590b

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

nickolaev approved these changes Dec 10, 2020

View reviewed changes

jakubdyszkiewicz reviewed Dec 10, 2020

View reviewed changes

lobkovilya added 5 commits December 14, 2020 17:01

fix(kuma-cp) move bugfix to separate branch

742b445

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Merge branch 'master' into fix/resource-conflict-error

89c84a1

fix(kuma-cp) after-merge fixes

d3b9929

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) after-merge fix

29ea203

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Merge branch 'master' into fix/resource-conflict-error

3100280

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # api/go.sum # go.sum # pkg/plugins/resources/k8s/native/go.sum # pkg/plugins/runtime/k8s/controllers/configmap_controller.go

jakubdyszkiewicz approved these changes Dec 23, 2020

View reviewed changes

fix(kuma-cp) review

fdb25fd

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya merged commit b348fb3 into master Dec 23, 2020

lobkovilya deleted the fix/resource-conflict-error branch December 23, 2020 13:59

mergify bot pushed a commit that referenced this pull request Dec 23, 2020

fix(kuma-cp) logging of resource conflicts (#1254)

921ccfc

(cherry picked from commit b348fb3)

mergify bot mentioned this pull request Dec 23, 2020

fix(kuma-cp) logging of resource conflicts (bp #1254) #1362

Merged

nickolaev pushed a commit that referenced this pull request Jan 4, 2021

fix(kuma-cp) logging of resource conflicts (#1254) (#1362)

8acd474

(cherry picked from commit b348fb3) Co-authored-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kuma-cp) logging of resource conflicts #1254

fix(kuma-cp) logging of resource conflicts #1254

lobkovilya commented Dec 2, 2020

jakubdyszkiewicz Dec 2, 2020

lobkovilya Dec 7, 2020

jakubdyszkiewicz Dec 2, 2020

lobkovilya Dec 7, 2020

jakubdyszkiewicz Dec 2, 2020

jakubdyszkiewicz Dec 2, 2020

lobkovilya Dec 7, 2020

jakubdyszkiewicz Dec 2, 2020

lobkovilya Dec 7, 2020

jakubdyszkiewicz Dec 2, 2020

lobkovilya Dec 7, 2020

jakubdyszkiewicz Dec 10, 2020

jakubdyszkiewicz Dec 10, 2020

lobkovilya Dec 11, 2020

lobkovilya Dec 14, 2020

jakubdyszkiewicz Dec 23, 2020

jakubdyszkiewicz Dec 23, 2020

fix(kuma-cp) logging of resource conflicts #1254

fix(kuma-cp) logging of resource conflicts #1254

Conversation

lobkovilya commented Dec 2, 2020

Summary

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment