search

TritonDataCenter / smartos-live

For more information, please see http://smartos.org/ For any questions that aren't answered there, please join the SmartOS discussion list: https://smartos.topicbox.com/groups/smartos-discuss
1.57k stars 246 forks source link

OS-7588 vmadm update fails to clear already cleared zonecfg properties #868

Closed joyent-automation closed 4 years ago

joyent-automation commented 4 years ago

OS-7588 vmadm update fails to clear already cleared zonecfg properties

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/5597/. The raw archive of this CR is here. See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@bahamas10 commented at 2019-02-15T21:35:21

Patch Set 1:

New commits:
commit b08809ef4a2b66c1cedc3f2d4171256b84e20dea
initial commit

sjorge commented 4 years ago

I opened #821 which triggered the creation of OS-7588. I did test this when this was first posted and it fixed the problem for me.

Would be great if this does make it in.

sjorge commented 4 years ago

@jlevon @papertigers what is missing for this? I retested it a few weeks ago and it still fixes the problem from #821 and it seems to have tests updates too?

jlevon commented 4 years ago

That's a question for @bahamas10 I think

jlevon commented 4 years ago

Happy to review if that would help speed things along though

bahamas10 commented 4 years ago

The code itself LGTM... I would just like to verify that all of the tests work as expected with this change in place, since a lot of code relies on this path.

edit: i'm an idiot as I see now I wrote this code and never merged it, whoops ha. I'll take a look at testing this this week.

jlevon commented 4 years ago

I ran vm tests against this change and it failed two of tests, test-vrrp-nics.js and test-send-recv.js

jlevon commented 4 years ago 
not ok 97 Error: write EPIPE
  ---
    type:    Error
    message: write EPIPE
    code:    ~
    errno:   ~
    file:    net.js
    line:    907
    column:  11
    stack:   
      - errnoException (net.js:907:11)
      - Object.afterWrite (net.js:723:19)
    wanted:  ~
    found:   ~
  ...
# delete after receiving kvm
not ok 98 error deleting VM: VM 5615e84a-396a-4cb3-c8e5-9e8e6521658e not found
  ---
    type:    AssertionError
    message: error deleting VM: VM 5615e84a-396a-4cb3-c8e5-9e8e6521658e not found
    code:    ~
    errno:   ~
    file:    /usr/vm/node_modules/VM.js
    line:    12005
    column:  21
    stack:   
      - /usr/vm/test/tests/test-send-recv.js:310:19
      - /usr/vm/node_modules/VM.js:12363:9
      - /usr/vm/node_modules/VM.js:12350:17
      - /usr/vm/node_modules/VM.js:12292:9
      - /usr/node/0.10/node_modules/async.js:240:13
      - /usr/node/0.10/node_modules/async.js:144:21
      - /usr/node/0.10/node_modules/async.js:237:17
      - /usr/node/0.10/node_modules/async.js:600:34
      - VminfodEventStream.<anonymous> (/usr/vm/node_modules/VM.js:12005:21)
    wanted:  true
    found:   false
  ...

1..98
jlevon commented 4 years ago

I re-ran the vrrp test twice it worked OK, so I don't think that comes from this change

twhiteman commented 4 years ago

FYI - those two failed tests (e.g. # delete after receiving kvm) are due to OS-8057.

jlevon commented 4 years ago

Thanks Todd. So I think this looks good. @bahamas10 want to merge?

sjorge commented 4 years ago

❤️ Thanks @jlevon and @bahamas10 to get this merged