[TEP-0135] Revert PVC creation #6893

QuanZhang-William · 2023-06-30T14:54:20Z

Part of #6740 and unblocks #6635. PVCs are created if a user specifies VolumeClaimTemplate as the source of a pipelinerun workspace. Prior to this change, such pvcs are created via affinity assistant statefulset when affinity assistant is enabled (in both workspaces or pipelineruns coschedule mode).

To delete such pvcs when the owning pipelinerun is completed, we must explicitly delete those pvcs in the reconciler since the owner of such pvcs is the affinity assistant statefulset instead of the pipelinerun. The problem of such deletion mechanism is that such pvcs are left in terminating state when the owning pipelinerun is completed but not deleted. This is because the pvcs are protected by kubernetes.io/pvc-protection finalizer, which does not allow the pvcs to be deleted until the pipelinerun consuming the pvc is deleted. Leaving pvcs in terminating state is confusing to cluster operators. Instead of changing the pvc deletion behavior in such backward incompatible way, it is better to make it configurable so that it is backward compatible, as prototyped in #6635.

This commit reverts the pvc creation behavior per-workspace coschedule mode, which changes the owner of the pvcs back to the pipelinerun instead of the affinity assistant statefulset. After the commit, the pvcs created by specifying VolumeClaimTemplate are left in bounded state instead of terminating. This commit is the prerequisite of #6635.

This commit does NOT reverts the pvc creation behavior per-pipelinerun coschedule mode as we will enforce the deletion of pvcs when the owning pipelinerun is completed. This is a better practice and there is no backward compatability concern since per-pipelinerun coschedule mode is a new feature.

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

TEP-0135: Revert the owner of `PVCs` created by `pipelinerun VolumeClaimTemplate` back to `pipelinerun`. The `PVCs` bounded to the `pipelinerun` is now in `bounded` state when the `pipelinerun` is completed but not deleted.

tekton-robot · 2023-06-30T14:54:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

QuanZhang-William · 2023-06-30T14:54:33Z

/test all

QuanZhang-William · 2023-06-30T14:57:45Z

/kind feature

tekton-robot · 2023-06-30T15:00:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	95.2%	-2.9
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.1%	-0.4

tekton-robot · 2023-06-30T15:02:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	95.2%	-2.9
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.1%	-0.4

tekton-robot · 2023-06-30T15:36:53Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	95.2%	-2.9
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.1%	-0.4

QuanZhang-William · 2023-06-30T17:06:51Z

cc @lbernick @pritidesai @afrittoli

pkg/reconciler/pipelinerun/affinity_assistant.go

pkg/reconciler/pipelinerun/pipelinerun.go

pkg/reconciler/pipelinerun/affinity_assistant_test.go

lbernick · 2023-07-05T11:57:15Z

pkg/reconciler/pipelinerun/affinity_assistant.go

@@ -88,7 +88,7 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsPerAABehavior(ctx context.C
 }
 for claimTemplate, workspaceName := range claimTemplatesToWorkspace {
 aaName := getAffinityAssistantName(workspaceName, pr.Name)
- err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, []corev1.PersistentVolumeClaim{*claimTemplate}, nil, unschedulableNodes)
+ err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{{ClaimName: claimTemplate.Name}}, unschedulableNodes)


It seems like this works because it assumes that pvcs have already been created from the volume claim template and that the function affinityAssistantStatefulSet only uses the volume claim templates in the statefulset spec. This is a bit hard to understand, and if we change affinityAssistantStatefulSet this will result in incorrect behavior. Maybe affinityAssistantStatefulSet needs to have some awareness of the affinity assistant mode, or we could use some explanatory comments + better variable naming of args passed into createOrUpdateAffinityAssistant?

This is a good point. I think affinityAssistantStatefulSet should have the sole responsibility to create an StatefulSet based on the given input (so usually we don't change it), and all our business logic should go to createOrUpdateAffinityAssistantPerAABehavior and createOrUpdateAffinityAssistant.

Let me try to find some better variable name in createOrUpdateAffinityAssistantsPerAABehavior and put some comments

I think the variable names are accurate, so I added some doc for this part, PTAL 😁 !

Here's how I would refactor this (involves rewriting CreatePVCsForWorkspacesWithoutAffinityAssistant):

case aa.AffinityAssistantPerWorkspace: ... for claimTemplate, workspaceBinding := range claimTemplatesToWorkspace { aaName := getAffinityAssistantName(workspaceBinding, pr.Name) pvc, err := c.pvcHandler.GetOrCreatePVC(workspaceBinding, pr) < handle err> err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{pvc}, unschedulableNodes) < handle err>

However, I know this PR is just meant to revert deletion behavior for the existing affinity assistant, and I want to keep this PR scoped to the minimal set of changes to do that. (I recognize I've already asked for some refactoring, but this chunk of code can be hard to grapple with :)) Maybe refactoring changes could be split into a separate PR?

Thanks for the advice! I think we can put the refactoring to another PR to keep this one scoped.

I'm approving this PR but I have a strong preference that refactoring changes happen first. typically refactoring changes that get deferred to later just get deprioritized.

Hi Lee, I did some investigation and realized that doing such refactoring in this PR will introduce a bunch of code changes. This makes this PR quite big and harder to review.

To address your concern, I have created an issue tracking it and assigned to myself: #6915 (will make sure to prioritize). Merging this PR as it is can help unblocking the followup PRs where I will add e2e support for coschedule pipelineruns mode, and I can work on the refactoring work in parallel (and the reviews for the followup PRs can happen in parallel).

WDTY?

Thank you! I appreciate it!

pkg/reconciler/pipelinerun/affinity_assistant.go

tekton-robot · 2023-07-05T15:29:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	98.1%	-0.1
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.1%	-0.4

tekton-robot · 2023-07-05T15:30:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	98.1%	-0.1
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.1%	-0.4

QuanZhang-William · 2023-07-05T21:25:52Z

/test pull-tekton-pipeline-beta-integration-tests

tekton-robot · 2023-07-05T21:26:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.2%	-0.3

QuanZhang-William · 2023-07-10T14:54:01Z

The reason we didn't catch this behavior change originally is due to a lack of integration tests, and I think the way k8s handles deletion of PVCs when the PV still exists would be hard to mock in a unit test without being very brittle. I think we should add integration tests that ensure PVCs are in the desired state after completion of the pipelinerun.

I think we add the integration test when we work on changing the PVC deletion behavior PR since the deletion behavior is pending change and we need to modify the integration test at time anyways.

I disagree-- the purpose of this PR is to change the deletion behavior, and I feel I'd have to test it out locally to ensure the PVCs are still in "bound" state on completion of the pipelinerun. This signals to me we need an integration test for this behavior. When the deletion behavior is modified, the tests can be easily modified as well; this will make it even easier to understand the impacts of the change.

SGTM, I have added an integration test to test the AA behavior from end to end. PTAL

tekton-robot · 2023-07-10T14:57:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.2%	-0.3

tekton-robot · 2023-07-10T14:58:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	91.5%	91.2%	-0.3

QuanZhang-William · 2023-07-10T15:04:54Z

/test pull-tekton-pipeline-integration-tests
status code 500 Internal Server Error when retrieving image

lbernick · 2023-07-10T15:09:54Z

test/affinity_assistant_test.go

+ knativetest "knative.dev/pkg/test"
+)
+
+func TestAffinityAssistant_PerWorkspace(t *testing.T) {


This looks great! Thanks Quan.

lbernick · 2023-07-10T15:12:19Z

pkg/reconciler/volumeclaim/pvchandler.go

-// When Affinity Assistant is enabled, the PersistentVolumeClaims will be created by the Affinity Assistant StatefulSet VolumeClaimTemplate instead.
+// This function is only called when the Affinity Assistant behavior is AffinityAssistantPerWorkspace or AffinityAssistantDisabled.
+// When the Affinity Assistant behavior is AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation,
+// the PersistentVolumeClaims will be created by the Affinity Assistant StatefulSet VolumeClaimTemplate instead.
 func (c *defaultPVCHandler) CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx context.Context, wb []v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error {


would it make sense to change this function name now?

SG, renamed it to CreatePVCsForWorkspaces as the usage of this function to create PVC means it is separated from Affinity Assistant StatefulSet

lbernick · 2023-07-10T15:13:58Z

pkg/reconciler/volumeclaim/pvchandler.go

+// This function is only called when the Affinity Assistant behavior is AffinityAssistantPerWorkspace or AffinityAssistantDisabled.
+// When the Affinity Assistant behavior is AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation,
+// the PersistentVolumeClaims will be created by the Affinity Assistant StatefulSet VolumeClaimTemplate instead.


I'm not a big fan of docstrings that explain how a function is called. I think it would be sufficient to explain that it creates PVCs for workspaces with volumeClaimTemplates

I have removed the usage docstring for now. Will further look into the explanation in the followup refactor PR

pkg/reconciler/pipelinerun/affinity_assistant_test.go

lbernick · 2023-07-10T15:16:22Z

pkg/reconciler/pipelinerun/affinity_assistant.go

@@ -88,7 +88,7 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsPerAABehavior(ctx context.C
 }
 for claimTemplate, workspaceName := range claimTemplatesToWorkspace {
 aaName := getAffinityAssistantName(workspaceName, pr.Name)
- err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, []corev1.PersistentVolumeClaim{*claimTemplate}, nil, unschedulableNodes)
+ err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{{ClaimName: claimTemplate.Name}}, unschedulableNodes)


I'm approving this PR but I have a strong preference that refactoring changes happen first. typically refactoring changes that get deferred to later just get deprioritized.

JeromeJu · 2023-07-10T15:45:33Z

/assign

tekton-robot · 2023-07-10T18:12:23Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	92.0%	91.8%	-0.3

tekton-robot · 2023-07-10T18:13:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	92.0%	91.8%	-0.3

JeromeJu · 2023-07-10T19:00:29Z

test/affinity_assistant_test.go

+ knativetest "knative.dev/pkg/test"
+)
+
+func TestAffinityAssistant_PerWorkspace(t *testing.T) {


nit: Thanks for the comments step by step in the e2e test. Wondering it might be beneficial to add a summary to the docstring here?

SGTM, docstring added

tekton-robot · 2023-07-10T19:28:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	92.0%	91.8%	-0.3

tekton-robot · 2023-07-10T19:29:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	92.0%	91.8%	-0.3

Yongxuanzhang · 2023-07-11T14:47:15Z

pkg/reconciler/pipelinerun/affinity_assistant_test.go

@@ -184,14 +185,16 @@ func TestCreateAndDeleteOfAffinityAssistantPerPipelineRun(t *testing.T) {

 // TestCreateAndDeleteOfAffinityAssistantPerWorkspace tests to create and delete an Affinity Assistant
 // per workspace for a given PipelineRun
-func TestCreateAndDeleteOfAffinityAssistantPerWorkspace(t *testing.T) {
+func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T) {


need to update the docstring for this test

Good catch, fixed!

Part of [tektoncd#6740] and unblocks [tektoncd#6635]. `PVCs` are created if a user specifies `VolumeClaimTemplate` as the source of a `pipelinerun workspace`. Prior to this change, such `pvcs` are created via `affinity assistant statefulset` when `affinity assistant` is enabled (in both `workspaces` or `pipelineruns` coschedule mode). To delete such pvcs when the owning `pipelinerun` is completed, we must explicitly delete those pvcs in the reconciler since the owner of such pvcs is the `affinity assistant statefulset` instead of the `pipelinerun`. The problem of such deletion mechanism is that such `pvcs` are left in `terminating` state when the owning `pipelinerun` is `completed` but not `deleted`. This is because the pvcs are protected by `kubernetes.io/pvc-protection` `finalizer`, which does not allow the `pvcs` to be deleted until the `pipelinerun` consuming the `pvc` is deleted. Leaving pvcs in `terminating` state is confusing to cluster operators. Instead of changing the pvc deletion behavior in such backward incompatible way, it is better to make it configurable so that it is backward compatible, as prototyped in [tektoncd#6635]. This commit reverts the pvc creation behavior `per-workspace` coschedule mode, which changes the owner of the `pvcs` back to the `pipelinerun` instead of the `affinity assistant statefulset`. After the commit, the pvcs created by specifying `VolumeClaimTemplate` are left in `bounded` state instead of `terminating`. This commit is the prerequisite of [tektoncd#6635]. This commit does NOT reverts the pvc creation behavior `per-pipelinerun` coschedule mode as we will enforce the deletion of pvcs when the owning `pipelinerun` is completed. This is a better practice and there is no backward compatability concern since `per-pipelinerun` coschedule mode is a new feature. [tektoncd#6740]: tektoncd#6740 [tektoncd#6635]: tektoncd#6635

tekton-robot · 2023-07-11T15:26:07Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	92.0%	91.8%	-0.3

tekton-robot · 2023-07-11T15:26:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	98.2%	97.2%	-1.0
pkg/reconciler/pipelinerun/pipelinerun.go	92.0%	91.8%	-0.3

JeromeJu

Thanks for answering the concerns on testing offline and adding the excellent commit message. Discussed couple of implementation choices with @QuanZhang-William offline.

/lgtm

tekton-robot · 2023-07-11T20:33:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu, lbernick, Yongxuanzhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2023

tekton-robot requested review from dibyom and imjasonh June 30, 2023 14:54

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 30, 2023

QuanZhang-William marked this pull request as ready for review June 30, 2023 15:30

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2023

tekton-robot requested a review from jerop June 30, 2023 15:30

lbernick reviewed Jun 30, 2023

View reviewed changes

pkg/reconciler/pipelinerun/affinity_assistant.go Show resolved Hide resolved

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2023

lbernick reviewed Jul 5, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0135-revert-pvc-creation branch from 6340ce2 to f88f1bf Compare July 5, 2023 15:23

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 5, 2023

QuanZhang-William force-pushed the tep-0135-revert-pvc-creation branch from f88f1bf to ee4c0c7 Compare July 5, 2023 21:21

lbernick approved these changes Jul 10, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2023

tekton-robot assigned JeromeJu Jul 10, 2023

QuanZhang-William mentioned this pull request Jul 10, 2023

Refactor CreatePVCsForWorkspacesWithoutAffinityAssistant #6915

Closed

QuanZhang-William force-pushed the tep-0135-revert-pvc-creation branch from aa65860 to 1ea210b Compare July 10, 2023 18:06

JeromeJu reviewed Jul 10, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0135-revert-pvc-creation branch from 1ea210b to b01af4a Compare July 10, 2023 19:22

Yongxuanzhang approved these changes Jul 11, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0135-revert-pvc-creation branch from b01af4a to e40338c Compare July 11, 2023 15:18

QuanZhang-William requested a review from Yongxuanzhang July 11, 2023 16:18

afrittoli added this to the Pipelines v0.50 (LTS) milestone Jul 11, 2023

afrittoli requested a review from pritidesai July 11, 2023 16:43

QuanZhang-William requested a review from JeromeJu July 11, 2023 17:40

JeromeJu approved these changes Jul 11, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2023

tekton-robot merged commit b769b56 into tektoncd:main Jul 11, 2023
2 checks passed

QuanZhang-William mentioned this pull request Jul 27, 2023

TEP-0135: Coscheduling PipelineRun pods Implementation #6740

Closed

[TEP-0135] Revert PVC creation #6893

[TEP-0135] Revert PVC creation #6893

Conversation

QuanZhang-William commented Jun 30, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 30, 2023

QuanZhang-William commented Jun 30, 2023

QuanZhang-William commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

QuanZhang-William commented Jun 30, 2023

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

tekton-robot commented Jul 5, 2023

tekton-robot commented Jul 5, 2023

QuanZhang-William commented Jul 5, 2023

tekton-robot commented Jul 5, 2023

QuanZhang-William commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

QuanZhang-William commented Jul 10, 2023

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

JeromeJu commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 11, 2023

tekton-robot commented Jul 11, 2023

JeromeJu left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 11, 2023

QuanZhang-William commented Jun 30, 2023 •

edited

Loading