Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic copying of changes to startup #6

Open
jktjkt opened this issue Apr 11, 2024 · 6 comments
Open

Automatic copying of changes to startup #6

jktjkt opened this issue Apr 11, 2024 · 6 comments

Comments

@jktjkt
Copy link
Contributor

jktjkt commented Apr 11, 2024

The standard says that:

If the NETCONF server supports :startup, the RESTCONF server MUST automatically update the non-volatile startup configuration datastore, after the "running" datastore has been altered as a consequence of a RESTCONF edit operation.

This essentially says that the changes which were done to the "default datastore" (when the NMDA URL paths are not used), should be propagated to startup as well. It will be fun to come up with all the rules for potential conflict solving when startup and running have diverged from each other.

@troglobit
Copy link

I just ran into this, and have been trying to get restconf/operations/ietf-netconf:copy-config to work in the meantime (to no avail so far), but maybe that's the easiest way forward also for this issue?

curl --http2-prior-knowledge -X POST -H "Content-Type: application/yang-data+xml" -H "Accept: application/yang-data+xml" -d '<ietf-netconf:copy-config xmlns:ietf-netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
  <source>
    <running/>
  </source>
  <target>
    <startup/>
  </target>
</ietf-netconf:copy-config>' "http://localhost:10080/restconf/operations/ietf-netconf:copy-config" -u admin:admin

@jktjkt
Copy link
Contributor Author

jktjkt commented May 30, 2024

I forgot that there's no <copy-config> copying between datastores in RESTCONF. The ietf-netconf:copy-config is a NETCONF-level operation which is handled by Netopeer2. Now, Netopeer2 does create a normal sysrepo-level RPC subscription, which means that one can invoke that RPC over RESTCONF already. However, there's some special filtering in Netopeer2 which means that any such RPC that has not originated through the NETCONF server gets rejected.

@troglobit, the problem is (IMHO) "hard" because it's a tricky question on how to solve potential conflicts. Sure, we could blindly do a copy from running to startup upon any change internally within rousette (that would be a oneliner to session.copyConfig(...)), but then there's a risk of overwriting stuff which was not supposed to be overwritten. Or, one can try to re-apply the same edit operation. However, in that case, what should happen when there's a failure in the second DS? In other words, we're ignoring this problem for now and are focusing on implementing the other features of the protocol.

@michalvasko: do you feel like relaxing np_ignore_rpc to also accept stuff like <copy-config> over RESTCONF?

@michalvasko
Copy link
Member

@jktjkt Not really and what is more, the plan is to change netopeer2 RPCs to work as they did originally, outside of sysrepo. The reason it was changed is so they can be subscribed by other applications, which are then at least notified when the RPC is executed. A notification can be generated in this case, which keeps the exact same functionality and will make netopeer2 simpler and faster.

@troglobit
Copy link

I forgot that there's no <copy-config> copying between datastores in RESTCONF. The ietf-netconf:copy-config is a NETCONF-level operation which is handled by Netopeer2. Now, Netopeer2 does create a normal sysrepo-level RPC subscription, which means that one can invoke that RPC over RESTCONF already. However, there's some special filtering in Netopeer2 which means that any such RPC that has not originated through the NETCONF server gets rejected.

Aha, I see. Did not know about that filtering thing, would've taken me a while to figure out. Thanks!

@troglobit, the problem is (IMHO) "hard" because it's a tricky question on how to solve potential conflicts. Sure, we could blindly do a copy from running to startup upon any change internally within rousette (that would be a oneliner to session.copyConfig(...)), but then there's a risk of overwriting stuff which was not supposed to be overwritten. Or, one can try to re-apply the same edit operation. However, in that case, what should happen when there's a failure in the second DS? In other words, we're ignoring this problem for now and are focusing on implementing the other features of the protocol.

Well, I don't claim to understand the complexities involved since I have very little insight into the protocols in play here. But I've worked a bit with other systems with similar concepts and from there the only real active configuration is the running-config, so on those systems, saving to startup-config is just a way of making sure the state is kept for next reboot. But I probably don't understand the problem fully yet, and I appreciate you all working on other more important parts of rousette! <3 We can do a local patch here in the meantime.

Cheers!

@jktjkt
Copy link
Contributor Author

jktjkt commented May 31, 2024

Under the NMDA, the startup and running are two "unrelated" datastores. Now, suppose that there's a YANG model like this:

container transmission {
  leaf mode {
    type enumeration {
      enum "auto";
      enum "manual";
    }
  }
  leaf current-gear {
    when "mode = manual";
    type int8 {
      range "1 .. 6";
    }
  }
}

What happens if startup has transmission/mode set to auto, and the running DS has transmission/mode set to manual, and the RESTCONF server is handling an edit which sets transmission/current-gear to 3? Sending "just this edit" to startup as well will surely fail. One could simply copy the config, but how much of the config? Just this module? That might break data integrity. So do we copy the entire DS content? We could, but then is is expected that copying, say, this change of gear also copies all unrelated changed which have been done recently by other clients, like, say, a tweak in a logging level somewhere, or this temporary SSH pubkey for root? And is that expected that this happens even if the server advertises full NMDA support, where the entire point is better control over how things are made persistent?

Honestly, I don't know what a good fix is.

@jktjkt
Copy link
Contributor Author

jktjkt commented Jun 3, 2024

Also, as per netconf-wg/restconf-next#16 it seems to me that this behavior might have been considered obsolete, or at least it might change in a future version of the standard. Maybe it's time to document our deviating behavior and call it a day (after a proper discussion on a mailing list, of course).

jktjkt added a commit that referenced this issue Sep 6, 2024
Since commit ca1ad2c, the HTTP client
which is used in the test for one-shot HTTP queries contained some code
which deferred HTTP error handling. The reason why this code was needed
is that when the request failed on the HTTP level, and if an exception
is thrown (that's what the `client.on_error(...)` call is doing), then
the usual rules of stack unwinding took effect, which meant that the
entire `client` variable went out of scope. Its destructor would then
try to call the request's `on_close(...)`, which was trying to shut down
the HTTP client, but that one was already being destroyed:

  ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f03e8cc60d0 at pc 0x565524c6a158 bp 0x7ffe0c7ab2d0 sp 0x7ffe0c7ab2c8
  READ of size 8 at 0x7f03e8cc60d0 thread T0
    #0 0x565524c6a157 in std::__shared_ptr<nghttp2::asio_http2::client::session, (__gnu_cxx::_Lock_policy)2>::get() const /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/shared_ptr_base.h:1666:16
    #1 0x565524c6a157 in std::__shared_ptr_access<nghttp2::asio_http2::client::session, (__gnu_cxx::_Lock_policy)2, false, false>::_M_get() const /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/shared_ptr_base.h:1363:66
    #2 0x565524c6a157 in std::__shared_ptr_access<nghttp2::asio_http2::client::session, (__gnu_cxx::_Lock_policy)2, false, false>::operator->() const /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/shared_ptr_base.h:1357:9
    #3 0x565524c6a157 in auto auto Response clientRequest<char const*, char const*>(char const*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, boost::posix_time::time_duration)::'lambda'(char const*)::operator()<boost::asio::ip::basic_resolver_iterator<boost::asio::ip::tcp>>(char const*) const::'lambda'(char const*)::operator()<unsigned int>(char const*) const CzechLight/rousette/tests/aux-utils.h:155:17
    #4 0x565524c6a157 in char const* std::__invoke_impl<void, auto Response clientRequest<char const*, char const*>(char const*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, boost::posix_time::time_duration)::'lambda'(char const*)::operator()<boost::asio::ip::basic_resolver_iterator<boost::asio::ip::tcp>>(char const*) const::'lambda'(char const*)&, unsigned int>(std::__invoke_other, char const*&&, unsigned int&&) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/invoke.h:61:14
    #5 0x565524c6a157 in std::enable_if<is_invocable_r_v<char const*, char const*, unsigned int>, char const*>::type std::__invoke_r<void, auto Response clientRequest<char const*, char const*>(char const*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, boost::posix_time::time_duration)::'lambda'(char const*)::operator()<boost::asio::ip::basic_resolver_iterator<boost::asio::ip::tcp>>(char const*) const::'lambda'(char const*)&, unsigned int>(char const*&&, unsigned int&&) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/invoke.h:111:2
    #6 0x565524c6a157 in std::_Function_handler<void (unsigned int), auto Response clientRequest<char const*, char const*>(char const*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, boost::posix_time::time_duration)::'lambda'(char const*)::operator()<boost::asio::ip::basic_resolver_iterator<boost::asio::ip::tcp>>(char const*) const::'lambda'(char const*)>::_M_invoke(std::_Any_data const&, unsigned int&&) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_function.h:290:9
    #7 0x7f03ecc55683 in std::function<void (unsigned int)>::operator()(unsigned int) const /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_function.h:591:9
    #8 0x7f03ecc55683 in nghttp2::asio_http2::client::request_impl::call_on_close(unsigned int) github/nghttp2/nghttp2-asio/lib/asio_client_request_impl.cc:67:5
    #9 0x7f03ecbf6daa in nghttp2::asio_http2::client::session_impl::~session_impl() github/nghttp2/nghttp2-asio/lib/asio_client_session_impl.cc:63:9
    #10 0x565524c40cd8 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release_last_use() /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/shared_ptr_base.h:175:2
    #11 0x7f03ecc0854e in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/shared_ptr_base.h:1071:11
    #12 0x7f03ecc0854e in std::__shared_ptr<nghttp2::asio_http2::client::session_impl, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/shared_ptr_base.h:1524:31
    #13 0x7f03ecc0854e in nghttp2::asio_http2::client::session_impl::do_read()::$_0::~$_0() github/nghttp2/nghttp2-asio/lib/asio_client_session_impl.cc:629:15
    #14 0x7f03ecc0854e in std::_Function_base::_Base_manager<nghttp2::asio_http2::client::session_impl::do_read()::$_0>::_M_destroy(std::_Any_data&, std::integral_constant<bool, false>) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_function.h:175:4
    #15 0x7f03ecc0854e in std::_Function_base::_Base_manager<nghttp2::asio_http2::client::session_impl::do_read()::$_0>::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_function.h:203:8
    #16 0x7f03ecc0854e in std::_Function_handler<void (boost::system::error_code const&, unsigned long), nghttp2::asio_http2::client::session_impl::do_read()::$_0>::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_function.h:282:6
    #17 0x7f03ecc1d18c in std::_Function_base::~_Function_base() /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_function.h:244:2
    #18 0x7f03ecc1d18c in boost::asio::detail::binder2<std::function<void (boost::system::error_code const&, unsigned long)>, boost::system::error_code, unsigned long>::~binder2() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/nghttp2-asio/include/boost/asio/detail/bind_handler.hpp:252:7
    #19 0x7f03ecc1d18c in boost::asio::detail::reactive_socket_recv_op<boost::asio::mutable_buffers_1, std::function<void (boost::system::error_code const&, unsigned long)>, boost::asio::any_io_executor>::do_complete(void*, boost::asio::detail::scheduler_operation*, boost::system::error_code const&, unsigned long) /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/nghttp2-asio/include/boost/asio/detail/reactive_socket_recv_op.hpp:155:3
    #20 0x565524c5b55e in boost::asio::detail::scheduler_operation::destroy() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/detail/scheduler_operation.hpp:45:5
    #21 0x565524c5b55e in void boost::asio::detail::op_queue_access::destroy<boost::asio::detail::scheduler_operation>(boost::asio::detail::scheduler_operation*) /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/detail/op_queue.hpp:47:8
    #22 0x565524c5b55e in boost::asio::detail::op_queue<boost::asio::detail::scheduler_operation>::~op_queue() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/detail/op_queue.hpp:81:7
    #23 0x565524c5b55e in boost::asio::detail::scheduler::abandon_operations(boost::asio::detail::op_queue<boost::asio::detail::scheduler_operation>&) /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/detail/impl/scheduler.ipp:447:1
    #24 0x565524c55f1f in boost::asio::detail::epoll_reactor::shutdown() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/detail/impl/epoll_reactor.ipp:92:14
    #25 0x565524c63f95 in boost::asio::detail::service_registry::shutdown_services() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/detail/impl/service_registry.ipp:44:14
    #26 0x565524c4a441 in boost::asio::execution_context::shutdown() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/impl/execution_context.ipp:41:22
    #27 0x565524c4a441 in boost::asio::io_context::~io_context() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/include/boost/asio/impl/io_context.ipp:58:3
    #28 0x565524c426c1 in Response clientRequest<char const*, char const*>(char const*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, boost::posix_time::time_duration) CzechLight/rousette/tests/aux-utils.h:165:1
    #29 0x565524c34d88 in DOCTEST_ANON_FUNC_4() CzechLight/rousette/tests/restconf-nacm.cpp:254:13
    #30 0x565524d65c92 in doctest::Context::run() /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/target/include/doctest/doctest.h:7007:21
    #31 0x565524d6ae96 in main /home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/target/include/doctest/doctest.h:7085:71
    #32 0x7f03ebd5d0cd in __libc_start_call_main (/nix/store/7jiqcrg061xi5clniy7z5pvkc4jiaqav-glibc-2.38-27/lib/libc.so.6+0x280cd) (BuildId: fd8eb9ded07dda31c46fd63a5351b53bac6a4f1d)
    #33 0x7f03ebd5d188 in __libc_start_main@GLIBC_2.2.5 (/nix/store/7jiqcrg061xi5clniy7z5pvkc4jiaqav-glibc-2.38-27/lib/libc.so.6+0x28188) (BuildId: fd8eb9ded07dda31c46fd63a5351b53bac6a4f1d)
    #34 0x565524aeed04 in _start (/home/jkt/work/prog/_build/czechlight-clang17-asan-ubsan/rousette/test-restconf-nacm+0x111d04)

I suspected that this is a bug in nghttp2-asio, but this turned out to
be just a plain bug in interaction between C++'s memory management and
ASIO, and the cure is to use weak_ptr which "sees" that the original
object is already gone.

Change-Id: I72ac8c5618ac339fcdd800398713667b20c17f4c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants