akondrahman / IaCTesting

Placeholder for the research study related to IaC testing anti-patterns
3 stars 256 forks source link

Does not follow a 'prepare' and 'verify' principle:::: RepoName: ansible-role-systemd_mount rep #4

Closed Talismanic closed 4 years ago

Talismanic commented 4 years ago

We have suspected that there is violation of 'prepare' and 'verify' principle . However, after further inspection I reached to the following decissions:

  1. test-create-btrfs-dev.yml, test-create-nfs-dev.yml, test-create-swap-dev.yml --> these three scripts are basically prepare steps. They created btrfs, nfs and swap mount points.
  2. Finally test.yml is the test script which verifies the creation of mount points.

So this is repo actually followed in the principle of 'prepare' and 'verify' principle.

akondrahman commented 4 years ago

Did you find this only one instance for prepare, then verify ?

Talismanic commented 4 years ago

No. There are some other suspected candidates for this anti-pattern. I am still examining those.

Talismanic commented 4 years ago

For example: ansible-role-python_venv_build\tests\test.yml was also a suspect for the violation of prepare & verify- however, after further examining, I decided that it did not violate the principle. It prepared the environment by installing nginx and python_venv and finally verified by Finding files and folders on the target.

akondrahman commented 4 years ago

All of these are good insights. Keep them posting :)

Talismanic commented 4 years ago

@akondrahman Bhai,

I am facing some difficulty in identifying this pattern in repo ansible-role-python_venv_build ([can be found here])(https://github.com/akondrahman/IaCTesting/blob/b82895f06f85108a76ca27fc3d83cc7c4da4b65b/categ_ansible_test_code.txt#L286), we can see it is following Prepare & Verify steps and both prepare and verify are in same playbook.

For the next case, in repo ansible-role-systemd_mount ([can be found here])(https://github.com/akondrahman/IaCTesting/blob/b82895f06f85108a76ca27fc3d83cc7c4da4b65b/categ_ansible_test_code.txt#L869) we can see that, it also followed prepare & verify, but in this case 'prepare' steps are in 3 different playbooks and verify step is in another playbook.

So from the yaml script, we can only identify whether it followed Prepare and Verify :

So, what should be our approach here?

One proposition can be:

  1. Check whether any playbook has been imported in the current playbook
  2. Parse all the task names and search for verify, check, assert, ensure, enabled etc.
  3. If found find a way to track relevant create, start, enable etc keyword in the other tasks or in the imported playbook

Problem is, again this will be a overfitting for our sample Bhaiya.

akondrahman commented 4 years ago

@Talismanic

I think the above-mentioned proposition is OK ... it will generate false positives but it will not miss anything. Our priority is to build TAMI in a manner so that nothing is missed. Once the paper is peer-reviewed we can focus on improving TAMI. So let's have a running tool that identifies all 6 categories.

Talismanic commented 4 years ago

@akondrahman Bhaiya, Till now I have been able to identify the task names which represents the "verify" step. Now to find the related "prepare" task name, I want to use nltk. Idea is, there is high probability that the task name of "verify" step and "prepare" step will have highest cosine similarity. For example:

Prepare Task Name : Show the ansible_local facts Verify Task Name : Verify that the ansible_local facts were set

Prepare Task Name : Show the files/folder from the targets Verify Task Name : Verify the folder list from the targets

Point is, whether using nltk will be costly here?

Sample Cosine Similarity Function

akondrahman commented 4 years ago

My instant feeling is no. It should not be expensive. So go ahead and use NLTK. After we implement we can time clock TAMI's execution performance.

Talismanic commented 4 years ago

However, result is not much satisfiable in my local machine. After running the above mecahnism, I found below output:

[{'preperationTask': 'Set venv_build_archive_path and venv_install_source_path', 'verifierTask': 'Verify that the facts were set'}, {'preperationTask': 'Compile the folder list from the targets', 'verifierTask': 'Verify the folder list from the targets'}, {'preperationTask': 'Set venv_build_archive_path and venv_install_source_path', 'verifierTask': 'Verify that the facts were set'}, {'preperationTask': 'Compile the folder list from the targets', 'verifierTask': 'Verify the folder list from the targets'}]

Seems 50% is success rate of identification.

akondrahman commented 4 years ago

Can you apply some heuristics to improve the performance so that the existing ones are detected?

Talismanic commented 4 years ago

@akondrahman Bhaiya, I need to study a bit how to apply heuristic in such conditions. I will update here after I make some improvements. If you have any blog/referrence, you can share with me.

akondrahman commented 4 years ago

@Talismanic

Sounds good. Can you show me 1-2 examples where your tool is misclassifying?

Talismanic commented 4 years ago

Example 1:

Here we can see that local facts has been refreshed. Which is prepare step.

https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L452

Then in the next step, it has been verified:

https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L457

However, the way our classifier is designed so far: code

  1. First it finds out all the task names of a playbook
  2. Then it detects which task names have the verification lead (search some keywords in the task name).
  3. Then we compare with each of the verifier task name with all other task name to find the prepare task name.

Instead of detecting refresh local facts task as the prepare step, it is detecting Set venv_build_archive_path and venv_install_source_path task which is in the below position.

https://github.com/akondrahman/IaCTesting/blob/b142283544331625455ab82aa447ae10dde1fb4c/categ_ansible_test_code.txt#L314

Talismanic commented 4 years ago

I have applied this with another script of systemd-mount repo. There also it is not giving good result. I think I have following errors in my previous assumtion:

  1. Semantic similarity is not a good measure of relationship like prepare and verifry.
  2. This is not even a causal relationship
  3. Rather this is some kind of sequential relationship. For example, if I do not prepare something, I should not be able to verify that.

So I am focussing on how to determine sequential relationship between two sentence from semantic analysis.

akondrahman commented 4 years ago

Not satisfied with the following heuristic ... keeping as a bookmark.

- check if prepare is used 
- check if verify is used 
- if verify is used then check if verify corresponds to the prepare used above ... for this see if the code element(s) appear for both the prepare block and verify block 
akondrahman commented 4 years ago

@Talismanic

Ignore the previous heuristic that I gave (I gave it a thumbs down :P). I think I need more information before I think about a heuristic:

  1. When you mentioned prepare then verify ... which artifact of Ansible do you refer to: downloading packages or collecting facts ... if you are lumping everything into one, then TAMI will misclassify as you are using one rule to detect them all. If my assumption is true, then tell me the subcategories and I can generate one rule for each.

  2. Both verify and prepare must operate some common code element like a fact or a downloaded package or sth else. So far how many code elements have you seen prepare and verify to be applicable? This is related to point#1.

I think it is better for me to see all sub-categories and corresponding examples ... may be you have it somewhere, which I missed.

Talismanic commented 4 years ago

@Talismanic

Ignore the previous heuristic that I gave (I gave it a thumbs down :P). I think I need more information before I think about a heuristic:

  1. When you mentioned prepare then verify ... which artifact of Ansible do you refer to: downloading packages or collecting facts ... if you are lumping everything into one, then TAMI will misclassify as you are using one rule to detect them all. If my assumption is true, then tell me the subcategories and I can generate one rule for each.

  2. Both verify and prepare must operate some common code element like a fact or a downloaded package or sth else. So far how many code elements have you seen prepare and verify to be applicable? This is related to point#1.

I think it is better for me to see all sub-categories and corresponding examples ... may be you have it somewhere, which I missed.

  1. Prepare step is where facts have been refreshed.

https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L452

Verify step is where facts have been verified.

https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L461

In most of the verify steps, some keywords are found (verify, ensure, check etc.). So our idea was first finding the verify step. Then finding whether there is any prepare step corresponding to that verify step using some semantic analysis.

  1. I do not have the exact count. But explicity I was working with 3 samples to verify our code.
akondrahman commented 4 years ago

@Talismanic

For the above-mentioned example, this what I would do:

  1. load the YAML file as key value pairs
  2. look for the setup key and get the ansible_local value
  3. check where ansible_local is referenced as part of assert and that
  4. if conditions satisfy then report prepare and verify to be detected

If you can present the other 3 samples, then I can look at them.

akondrahman commented 4 years ago

From your code here https://github.com/akondrahman/IaCTesting/commit/b142283544331625455ab82aa447ae10dde1fb4c I noticed you are not loading the YAML files as a list of key value pairs ... is that a correct interpretation?

Talismanic commented 4 years ago

From your code here b142283 I noticed you are not loading the YAML files as a list of key value pairs ... is that a correct interpretation?

Bhaiya, I have used python's yaml library to read the yaml file as a key-value pair and used that as a json object. This loading is done in the below file.

https://github.com/akondrahman/IaCTesting/blob/b142283544331625455ab82aa447ae10dde1fb4c/detecting_prepare_and_execute_violation.py#L17

Talismanic commented 4 years ago

@Talismanic

For the above-mentioned example, this what I would do:

  1. load the YAML file as key value pairs
  2. look for the setup key and get the ansible_local value
  3. check where ansible_local is referenced as part of assert and that
  4. if conditions satisfy then report prepare and verify to be detected

If you can present the other 3 samples, then I can look at them.

Example 01

Prepare Step: https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L452

Verify Step: https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L461

Example 02

Prepare Step: https://github.com/akondrahman/IaCTesting/blob/25e01b34d347d1ed99051522500e5fe595f2b58b/categ_ansible_test_code.txt#L701

Verify Step: https://github.com/akondrahman/IaCTesting/blob/25e01b34d347d1ed99051522500e5fe595f2b58b/categ_ansible_test_code.txt#L904

Example 03

Prepare Step: https://github.com/akondrahman/IaCTesting/blob/25e01b34d347d1ed99051522500e5fe595f2b58b/categ_ansible_test_code.txt#L851

Verify Step: https://github.com/akondrahman/IaCTesting/blob/25e01b34d347d1ed99051522500e5fe595f2b58b/categ_ansible_test_code.txt#L912

Talismanic commented 4 years ago

@Talismanic Ignore the previous heuristic that I gave (I gave it a thumbs down :P). I think I need more information before I think about a heuristic:

  1. When you mentioned prepare then verify ... which artifact of Ansible do you refer to: downloading packages or collecting facts ... if you are lumping everything into one, then TAMI will misclassify as you are using one rule to detect them all. If my assumption is true, then tell me the subcategories and I can generate one rule for each.
  2. Both verify and prepare must operate some common code element like a fact or a downloaded package or sth else. So far how many code elements have you seen prepare and verify to be applicable? This is related to point#1.

I think it is better for me to see all sub-categories and corresponding examples ... may be you have it somewhere, which I missed.

  1. Prepare step is where facts have been refreshed.

https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L452

Verify step is where facts have been verified.

https://github.com/akondrahman/IaCTesting/blob/b7e622bcaff7b177905cb0c3677e65233a966513/categ_ansible_test_code.txt#L461

In most of the verify steps, some keywords are found (verify, ensure, check etc.). So our idea was first finding the verify step. Then finding whether there is any prepare step corresponding to that verify step using some semantic analysis.

  1. I do not have the exact count. But explicity I was working with 3 samples to verify our code.

You are right bhaiya about point 1. I was trying to generalize all the 'prepare' and 'verify' steps.

akondrahman commented 4 years ago

My feedback on the three examples:

Example 03

This does not look like a valid not following prepare and verify to me ... you are basing your detection heuristic based on the keyword swap , which is done differently in the prepare step and the verify step. One is a swap file, one is a swapon command. I would not count this as a valid not following prepare and verify and I would advice you do to the same.

Example 02

This does not look like a not following prepare and verify ... I think you are focusing on /var/lib that does not match properly later.

Example 01

This looks like following prepare and verify as ansible_local is being set and then verified using assert ... I suggest you take Example 01 as an example of following prepare and verify then determining not following prepare and verify will be easy. Based on our discussion we will see a lot of examples for not following prepare and verify

akondrahman commented 4 years ago

@Talismanic

Follow up question: do you think skip_ansible_lint implies skipping ansible lint checks? This link says

A less-preferred method of skipping is to skip all task-based rules for a task (this does not skip line-based rules). There are two mechanisms for this: the skip_ansible_lint tag works with all tasks, and the warn parameter works with the command or shell modules only.

Makes me wonder if skipping ansible lint is an anti-pattern as we have seen checking coding anti-patterns as a recommended practice from our LANGETI paper.

Talismanic commented 4 years ago

@Talismanic

Follow up question: do you think skip_ansible_lint implies skipping ansible lint checks? This link says

A less-preferred method of skipping is to skip all task-based rules for a task (this does not skip line-based rules). There are two mechanisms for this: the skip_ansible_lint tag works with all tasks, and the warn parameter works with the command or shell modules only.

Makes me wonder if skipping ansible lint is an anti-pattern as we have seen checking coding anti-patterns as a recommended practice from our LANGETI paper.

Bhaiya, I was thinking the same last week. But put it aside keeping a note in my notebook to check this in details later and focussed on the prepare-verify identification.

Talismanic commented 4 years ago

My feedback on the three examples:

Example 03

This does not look like a valid not following prepare and verify to me ... you are basing your detection heuristic based on the keyword swap , which is done differently in the prepare step and the verify step. One is a swap file, one is a swapon command. I would not count this as a valid not following prepare and verify and I would advice you do to the same.

Example 02

This does not look like a not following prepare and verify ... I think you are focusing on /var/lib that does not match properly later.

Example 01

This looks like following prepare and verify as ansible_local is being set and then verified using assert ... I suggest you take Example 01 as an example of following prepare and verify then determining not following prepare and verify will be easy. Based on our discussion we will see a lot of examples for not following prepare and verify

What I am doing now, is listing all the prepare+verify steps I can find in this spreadsheet. Then I will do a cross verification with you whether those actually corresponds to prepare+verify. Then we will put some label on those. Then we will decide how to deal with those.

akondrahman commented 4 years ago

Sounds like a plan.

Talismanic commented 4 years ago

@akondrahman Bhai, In this function, we are seeing that, config file is being loaded:

https://github.com/akondrahman/IaCTesting/blob/25e01b34d347d1ed99051522500e5fe595f2b58b/categ_ansible_test_code.txt#L1694

And in the next line, we are seeing that some part of the loaded_config file is being checked against some other values. https://github.com/akondrahman/IaCTesting/blob/25e01b34d347d1ed99051522500e5fe595f2b58b/categ_ansible_test_code.txt#L1695

Should we consider first step as prepare and second step as verify?

Talismanic commented 4 years ago

I am still searching how to deduce the relationship. While searching, I found an interesting paper and learnt an idea called Preconditional Anaphoric Relationship. This might not help us in this directly, but the idea was new to me. So kept it posted here just for future referrence.

Talismanic commented 4 years ago

Also posted one question on stackexchange to check if anybody from datascience community can help.

Talismanic commented 4 years ago

@akondrahman Bhaiya, I have tried to simplified the thought process in this anti-pattern. For Ansible files, I planned for below:

  1. Identifying the assert statements
  2. Extracting the variables from assert
  3. Checking whether those variable has been set properly before assert.

For example below is one assertion block from ansible-role-systemd_networkd repo.

 tasks:
    - name: Interface check
      assert:
        that:
          - ansible_dummy0['active'] == true
          - ansible_dummy0['type'] == 'ether'
          - ansible_dummy0['mtu'] == 9000
          - ansible_dummy1['active'] == true
          - ansible_dummy1['type'] == 'ether'
          - ansible_dummy1['mtu'] == 9000
          - ansible_dummy2['active'] == true
          - ansible_dummy2['type'] == 'ether'
    - name: Bond check
      assert:
        that:
          - ansible_bond0['active'] == true
          - ansible_bond0['type'] == 'bonding'
          - ansible_bond0['mtu'] == 9000

But when I am trying to extract the variables and check whether they have been set properly, I am seeing the setter portion is like below:

systemd_netdevs:
      - NetDev:
          Name: dummy0
          Kind: dummy
      - NetDev:
          Name: dummy1
          Kind: dummy
      - NetDev:
          Name: bond0
          Kind: bond
        Bond:
          Mode: 802.3ad
          TransmitHashPolicy: layer3+4
          MIIMonitorSec: 1s
          LACPTransmitRate: fast
      - NetDev:
          Name: br-dummy
          Kind: bridge
      - NetDev:
          Name: dummy2
          Kind: dummy
      - NetDev:
          Name: br-test
          Kind: bridge
    systemd_networks:
      - interface: "dummy0"
        bond: "bond0"
        mtu: 9000
      - interface: "dummy1"
        bond: "bond0"
        mtu: 9000
      - interface: "bond0"
        bridge: "br-dummy"
        mtu: 9000
      - interface: "br-dummy"
        address: "10.0.0.100"
        netmask: "255.255.255.0"
        gateway: "10.0.0.1"
        mtu: 9000
        usedns: true
        static_routes:
          - gateway: "10.1.0.1"
            cidr: "10.1.0.0/24"
        config_overrides:
          Network:
            ConfigureWithoutCarrier: true
      - interface: "dummy2"
        bridge: "br-test"
      - interface: "br-test"
        address: "10.1.0.1"
        netmask: "255.255.255.0"

So the setters are too much spread out in different parts of the code. I am a bit confused how to deal with this. Any suggestion Bhaiya?

Talismanic commented 4 years ago

Or this whole approach is too naive? If there is an assert, there must be an assignment of variable, there's nothing to check actually.

akondrahman commented 4 years ago

Seems like detection of prepare and verify will be hard to do as there is too much semantics involved that is hard to detect through static code analysis. I suggest you drop this anti-pattern and try to see violations of practices that you listed in your workshop paper.

We have identified one only doing localhost testing. Can we identify more?

Talismanic commented 4 years ago

We have identified one only doing localhost testing. Can we identify more?

If we think about our best practices, below is my initial assessment:

Use of Automation:

For simplicity we can consider the use of molecule as the indicator of using automation. For this purpose, we can traverse the L1/L2/test directory of the base project. If molecule is used, framework normally creates a directory named molecule. If we can find that directory, we can assume that molecule has been used. This is a very simplistic approach and might be prone to false positive (where only directory is there, but no testing is automated by molecule).

For the practitioners who use tox to automate the test cases written in python, we need to check whether tox.ini contains at least 1 envlist, commands of those envs and there are at least some test codes in the TESTS_DIR mentioned in the tox file. It might be a little bit complicated, but its more accurate and doable.

Sandbox Testing:

I think its not identifiable from code base whether the environment is isolated or not accurately. Analysing the code base it seemed to me, we can obtain environment information from hosts parameter of ansible code. But this parameter does not give clear indication where the environment is a sandboxed environment or not.

Testing every IaC Change:

One way of determining this is to search for a ci indicative file. For example, searching for '.travis' , '.circleci' or 'jenkinsfile' in the project root. However, this will tell us the project has CI integration. But to accurately check whethere every change is being tested we need to analyse the co-evolution of original source and test code. I think that is a bigger doamin to analyse as we need to check this from git commit point of view, not file point of view. Good part is, not finding any ci indicative file will at least give the hint to practitioners that they should integrate this CI.

Behavior Focused Testing:

Here I faced the similar dillema of 'prepare and verify'. Understanding which test case is testing which behavior will take too much semantic analysis.

Avoiding Coding Anti-Patterns:

This is partially covered in 'skip_ansible_linting'. If linting is enabled, linter will find the smells, anti-patterns etc.

Remote Testing:

Covered under localhosting.

Please let me know your views on these.

akondrahman commented 4 years ago

Acknowledged. I totally agree with you.

Can you please see if any of Table 4 from the following paper can be found in the Ansible test scripts? icsme15-testbugs.pdf

Talismanic commented 4 years ago

Can you please see if any of Table 4 from the following paper can be found in the Ansible test scripts? icsme15-testbugs.pdf

On a high level, I could not find any in the ansible test scripts (both in yaml and python). Looks like those are more related to software test code.

Talismanic commented 4 years ago

Acknowledged. I totally agree with you.

I am starting to use of automation and not finding this will be considered as an antipattern.

akondrahman commented 4 years ago

Look at randomly selected 100 scripts first. If you can't find any then probably there is none. We can spend on the above-mentioned activity for 2-3 days, not more than that.

Talismanic commented 4 years ago

Look at randomly selected 100 scripts first. If you can't find any then probably there is none. We can spend on the above-mentioned activity for 2-3 days, not more than that.

Bhaiya, we have 14 ansible projects. I checked and found each of them used tox.ini to automate testing to some extent.

Only 1 project has molecule directory. But that does not have any significate files.

From individual scripts it is not identifiable whether whole project implemented any test automation or not.

Should I proceed based on the tox files and presence of molecule folder?

akondrahman commented 4 years ago

Should I proceed based on the tox files and presence of molecule folder? It is very hard to conclude that an Ansible repository to not use any automated testing. I suggest you abandon this idea.

However, is it possible to say whether or not testing is being done for an Ansible role? Can you find some evidence on this from the Openstack repos that I gave you?

Talismanic commented 4 years ago

Should I proceed based on the tox files and presence of molecule folder? It is very hard to conclude that an Ansible repository to not use any automated testing. I suggest you abandon this idea.

However, is it possible to say whether or not testing is being done for an Ansible role? Can you find some evidence on this from the Openstack repos that I gave you?

Bhaiya, I searched for 3 repos from Openstack:

  1. browbeat
  2. Ironic-lib
  3. openstack ansible

I might be wrong, but it seems whether any ansible role has been testing automatically can not be determined properly. At least I could not match any of the playbook file to any of the test scripts one2one till now. I am still going through these.

Even if we can do this, that will be another anti-pattern as test scripts are meant to test the behavior of the whole playbook or project.

akondrahman commented 4 years ago

I suggest we abandon this. Are you done with the tool prep?

Talismanic commented 4 years ago

Are you done with the tool prep?

Yes. I think tool is in shape to be used to detect the antipatterns.

akondrahman commented 4 years ago

I think it is time to mine repositories. See issue https://github.com/akondrahman/IaCTesting/issues/14. Also please call issues that are no longer relevant.

Talismanic commented 4 years ago

Closing this issue as we discarded this anti-pattern